Making parameter files JSON-compliant

[davidbbeyer]

Addresses #5

@pm-blanco I have made the pKa-sets JSON compliant, as suggested in #5. Furthermore, I have added a new CI-test testsuite/parameter_test.py to check that the different pKa-sets are correctly formatted. If you agree with the overall conception, I would go ahead and do the same thing for the peptide force-fields/interaction parameters.

[pm-blanco]

The design looks pretty good to me, I would only add a "citekey" additional key to the metadata dictionary with a label for each data set, for the perspective feature to add a cite method in pyMBE. Maybe @jngrad would be a better reviewer for this PR since this was his suggestion :smile:

[pm-blanco]

Great! Then once @davidbbeyer finishes refactoring the rest of data for the peptide parameters we can proceed to merge this PR.

[davidbbeyer]

I already added the citekeys now...

[pm-blanco]

@davidbbeyer @jngrad then maybe we can leave the citekey logic, it has the advantage that allows to search for the bittex item more easilly than having to search for either an isbn or a doi. The CI has somehow broken in the last commit, any idea of why is the artifact building the documention not working anymore?

[davidbbeyer]

Could it maybe related to the naming of the new folder /docs?

[pm-blanco]

@davidbbeyer yes, it seems that it is a keyword in Github for the folder with the source to publish the website for the github page of our project here. Maybe simply rename it to refs?

[jngrad]

Could it maybe related to the naming of the new folder /docs?

Yes. I removed .PHONY: docs in a5241627896, and in subsequent commits more phony targets were removed.

I don't know if there is any authoritative "Makefile best practices". A quick search reveals multiple such best practice guides from various sources, but only one mentions PHONY and its caveats (https://unix.stackexchange.com/a/217308).

I wouldn't be opposed to re-introducing phony targets. I originally removed it because it was a bit confusing to me to do "make docs" when the "docs" folder already exists. In fact, with phony targets it is super easy to accidentally make a folder like "docs" act as both an input and output of a Makefile target.

In ESPResSo we solved that issue by using CMake, which hides all this Makefile logic for us. We also use "make sphinx", "make doxygen" and "make tutorials" rather than "make docs", something that pyMBE might actually want to do in the future once we have Jupyter Notebooks and decide to deploy them to the GitHub Pages.

[pm-blanco]

For the sake of this PR, I suggest that for now simply rename the folder because naming the folder with the bibtex file as docs was probably not a very fortunate choice anyway in the first place because it conflicts with the folder documentation where we make the API documentation of pyMBE. I would simply rename the folder to another more clear name and not reintroduce the PHONY targets again. Moving to CMake in the future is an interesting idea, it would be nice to have the tutorials as gitlab pages.

[jngrad]

Moving to CMake in the future is an interesting idea, it would be nice to have the tutorials as gitlab pages.

Sorry, my original message was a bit ambiguous! I meant to say that pyMBE might be interested in creating more fine-grained Makefile targets like "sphinx" and "tutorials" in the future. It is probably too early to introduce CMake in pyMBE.

[davidbbeyer]

I have also refactored the peptide interaction parameters now.

[pm-blanco]

@davidbbeyer @jngrad sorry for messing around with the almost ready for merge PR, but I noticed that the CI was having quite a lot of verbose and I could not help myself to clean it up before it propagates further. I also noticed that the documentation on \parameters/peptides/Lunkad2021_Avogadro.json is not correct. That file corresponds actually to a mix of parameters taken from Lunkad2021a and some values parametrized by Peter's master student using Avogadro. It was kept in pyMBE because I promised her not to remove it until her master thesis was done, but I think that at this stage it can be safely removed.

[davidbbeyer]

@pm-blanco Good to know, I saw that in the ESI of the Lunkad paper it was mentioned that there is an alternative way to parametrize the peptides that uses the Avogadro software, so I assumed it was that.

[jngrad]

but I noticed that the CI was having quite a lot of verbose and I could not help myself to clean it up before it propagates further.

Thanks for reducing the verbosity level. I am currently working on disabling the PSM3-related warnings from the CI pipeline log files to make them more readable. Maybe I can get this ready by the end of the week (in a separate PR).

I also played around with pytest-xdist to run tests concurrently and halve the CI runtime (GitHub Actions have 2 cores). The workflow would be pytest -n 2 testsuite/ to automatically discover tests and run them on 2 cores via a scheduler, or pytest -d --tx 2*popen//python=python3.10 testsuite/ to open 2 subprocesses. Unfortunately pytest seems to run the tests twice, which leads to runtime errors since we can't have more than one ESPResSo system. I'm more familiar with unittest the CTest scheduler from the CMake suite. If you have any idea why pytest runs the code twice and how to avoid it (need a if __name__ == "__main__"? need a class singleton?), let me know!

[pm-blanco]

@jngrad I have never used pytest-xdist to run tests in paralel, but while doing a quick search on the topic I found the following discussion on the topic here, might be useful?

[jngrad]

@jngrad I have never used pytest-xdist to run tests in paralel, but while doing a quick search on the topic I found the following discussion on the topic here, might be useful?

hmm... not quite. I now think the issue is with how pytest executes a single test. Maybe it imports it twice to gather more information about the traceback of failed checks. I don't really have the time to look into it further. ESPResSo will support multi-system simulations eventually anyway.

We also cannot really halve the runtime, because most of the time spent in the testsuite is due to 2 or 3 titration tests.

[pm-blanco]

@jngrad I have never used pytest-xdist to run tests in paralel, but while doing a quick search on the topic I found the following discussion on the topic here, might be useful?

hmm... not quite. I now think the issue is with how pytest executes a single test. Maybe it imports it twice to gather more information about the traceback of failed checks. I don't really have the time to look into it further. ESPResSo will support multi-system simulations eventually anyway.

We also cannot really halve the runtime, because most of the time spent in the testsuite is due to 2 or 3 titration tests.

Yes, I do not think that is worth expending more time on trying to paralelize the CI at least for now, we should rather trying to optimize the titration tests that are more costly.