👌 Adding validators to the extra tags

[JPchico]

Adding the publication_year, developer and title to the list of required parameters when one is creating the LammpsPotentialData node.

The idea behind this is to make sure that the potentials are more easily queriable, and the developer name, title and year are amongst the most common of the tags that one would use to try to get the potential from the database.

Fixes #72

[codecov]

Codecov Report

Merging #76 (bdbd52d) into develop (b742605) will increase coverage by 1.43%.
The diff coverage is 78.46%.

@@             Coverage Diff             @@
##           develop      #76      +/-   ##
===========================================
+ Coverage    87.36%   88.80%   +1.43%     
===========================================
  Files           17       17              
  Lines         1369     1384      +15     
===========================================
+ Hits          1196     1229      +33     
+ Misses         173      155      -18     

Flag	Coverage Δ
pytests	88.80% <78.46%> (+1.43%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiida_lammps/fixtures/data.py	100.00% <ø> (ø)
aiida_lammps/data/potential.py	79.06% <75.43%> (-2.40%)	⬇️
aiida_lammps/parsers/inputfile.py	87.23% <100.00%> (-0.08%)	⬇️
aiida_lammps/validation/__init__.py	100.00% <100.00%> (+100.00%)	⬆️
aiida_lammps/validation/utils.py	100.00% <100.00%> (+100.00%)	⬆️

[sphuber]

Should this really become required? If the only reason is that having this information makes it more queryable, what if a user doesn't care about querying for this information? They will now however be forced to always enter this information. Feels like this may lead to unnecessary frustration? What is the problem with keeping it optional and those users that would like to be able to query for it, do so?

Also, I noted that the class was based off of the PseudoData class I wrote for aiida-pseudo (my name was misspelled in the attribution in the docstring 😅 ) but the things that were added are not quite correct. But before addressing those, thought I'd first discuss this first point mentioned above.

[JPchico]

Should this really become required? If the only reason is that having this information makes it more queryable, what if a user doesn't care about querying for this information? They will now however be forced to always enter this information. Feels like this may lead to unnecessary frustration? What is the problem with keeping it optional and those users that would like to be able to query for it, do so?

I was discussing this with my co-workers who use LAMMPS more than me, and in their opinion these attributes are quite key when using the potential. I agree that their only function is making the potential more queriable, but in the case of lammps maybe that is okay? Since we do not have things as potential families, we need to have a set of things that allows us to differentiate potential from potential.

From my perspective, all the information including the extras should always be provided, as you want to know from where this potential comes form. But I agree that this is not feasible and will lead to frustration (it would be so much simpler if the potential files themselves would have the metadata, but that is an issue beyond what we can control sadly). The question I guess is what is the minimal set that should be required?

Also, I noted that the class was based off of the PseudoData class I wrote for aiida-pseudo (my name was misspelled in the attribution in the docstring sweat_smile ) but the things that were added are not quite correct. But before addressing those, thought I'd first discuss this first point mentioned above.

Ooups that is my bad sweat_smile, I'll fix that up. Yes I based it on the aiida-pseudo after some discussions with you in the aiida mailing list. I think it is the best way of treating this as we do not have the potential families. I guess that one could make families for EAM for example, but you could have many EAM potentials for a given element depending on how it was parametrized. So this was the best solution we could find.

[sphuber]

(it would be so much simpler if the potential files themselves would have the metadata, but that is an issue beyond what we can control sadly)

Isn't this exactly an indication that even when people are running LAMMPS outside of AiiDA they also don't always strictly include this kind of metadata? This makes sense if you're developing or just testing. You don't really care about this kind of supplementary information unless you are doing production work maybe.

The question I guess is what is the minimal set that should be required?

I would say, whatever is needed to actually make the calculation run. Only if the data missing would cause the calculation to fail, would I make the data required.

In general, I think a plugin should put as little unnecessary "barriers" as possible. This is also the very reason I added the input to provide a complete input scripts, as opposed to forcing a user to go through the AiiDA specific inputs. Sure, the latter may be useful in certain cases (again it improves mostly queryability) but it is in no means necessary to run LAMMPS through AiiDA, and making it impossible to do so will just force people to stop using the plugin altogether.

[JPchico]

Isn't this exactly an indication that even when people are running LAMMPS outside of AiiDA they also don't always strictly include this kind of metadata? This makes sense if you're developing or just testing. You don't really care about this kind of supplementary information unless you are doing production work maybe.

Sure, the fact that there is not a single well established format for the metadata is an indication that not everyone uses it. Of course we based ourselves in the OpenKIM specification, which is aims to solve this problem. All the parameters that are stipulated here in principle are part of the specification.

I would say, whatever is needed to actually make the calculation run. Only if the data missing would cause the calculation to fail, would I make the data required.

In general, I think a plugin should put as little unnecessary "barriers" as possible. This is also the very reason I added the input to provide a complete input scripts, as opposed to forcing a user to go through the AiiDA specific inputs. Sure, the latter may be useful in certain cases (again it improves mostly queryability) but it is in no means necessary to run LAMMPS through AiiDA, and making it impossible to do so will just force people to stop using the plugin altogether.

In this I agree, the plugin should let the user run the calculation in the simplest way possible. That is a big part of the whole refactoring that we have been doing. The plugin was too restrictive, only allowing certain things to be done.

I think that queriability is quite important though, finding your data, being able to do meta analysis, and share are crucial. However, the question is how much of that burden should be placed in the plugin and how much in the user. Forcing the user to store a set of parameters might be too much.

[JPchico]

@sphuber After some discussions I think that indeed we can remove mostly of what is on this PR, i.e. adding the developer, title and publication_year can be removed, and instead some of the other minor fixes that were added here can be left.

And I can correct the misspelling of your name 😅

[JPchico]

@sphuber I have amended this PR, so now it just adds some validators for the extra tags, so that if they are provided they are checked properly.

I also fixed the misspelled name