Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature web (a.k.a. BioSimSpace 2023.0.0) #356

Merged
merged 59 commits into from
Sep 26, 2022
Merged

Feature web (a.k.a. BioSimSpace 2023.0.0) #356

merged 59 commits into from
Sep 26, 2022

Conversation

chryswoods
Copy link
Member

This is the update of BioSimSpace that is compatible with Sire 2023.0.0.

The changes are minimal, i.e. updating to using the new (PEP8) module names of sire (but not the PEP8 function names - hence why sire.use_mixed_api() is used).

The other changes are to the build system. This now mirrors sire's updated build system. This supports building Windows packages, plus gives more control over the dependency relationship between BioSimSpace and Sire.

This won't pass its tests until Sire 2023.0.0 is available.

Christopher Woods and others added 21 commits June 10, 2022 18:34
…nge :-)

Just a few errors in the tests in test_somd.py and test_generalunit.py

I suspect these may be because I haven't yet pulled in any changed from
Sire devel to feat_web. I'll get around to this next week :-)
…o run shlex.split properly on windows. Debugging unit errors
… command lines. Switched out shlex.split for this new function
…es_num() as this is loading

a system from a pickled file, and the residue number is not correct.
… edge case bug

caused by using both Sire.Mol and sire.legacy.Mol.

Have some small edge cases to deal with now

FAILED test/Process/test_somd.py::test_pert_file[test/input/morphs/morph01.pickle-test/input/morphs/morph01.pert] -
ModuleNotFoundErr...
FAILED test/Process/test_somd.py::test_pert_res_num - ModuleNotFoundError: No module named 'Sire'
FAILED
test/Sandpit/Exscientia/Process/test_somd.py::test_pert_file[test/Sandpit/Exscientia/input/morphs/morph01.pickle-test/Sandpit/Exscientia/input/morphs/morph01.pert]
FAILED test/Sandpit/Exscientia/Process/test_somd.py::test_pert_res_num - ModuleNotFoundError: No module named 'Sire'
FAILED test/Sandpit/Exscientia/_SireWrappers/test_molecule.py::test_makeCompatibleWith - ValueError: 'Invalid search query:
'element F'
FAILED test/_SireWrappers/test_molecule.py::test_makeCompatibleWith - ValueError: 'Invalid search query: 'element F'

The test_somd failures are because we now can't unpickle a file that expects to load Sire instead of sire

The 'element F' failure is unexpected and will be debugged later
…turned zero results

would not raise an exception. I have fixed this for Molecule, but it will likely need
fixing for others.

I've marked the pickle tests in test_somd.py as XFAIL, as we will need to create a
new pickle object for this to work with the new version of Sire.

Otherwise, everything now passes.
…dows.

Updated to the same requirements system as sire, specifying all requirements
in `requirements.txt` and `requirements_build.txt`. These are parsed
by `update_recipe.py` to create the conda recipe.

I have moved the sire version requirement into requirements.txt so that
it is easier to specify the sire version on which a pull request or
push depends. This way we don't need to be playing around with the
tags or anything. This also lets us control the range of supported
sire versions more easily, e.g. not having to use the major number
as the pin.

I have also updated the build process so that we don't need to
clone the sire or biosimspace repositories again - for the tests
we just copy the tests from the already-cloned repo.

Note that this won't build an run now because it depends
on sire >= 2023.0.0, < 2023.1.0
@lohedges
Copy link
Member

lohedges commented Sep 22, 2022

Okay, I've made a few edits to remove some redundant imports and tidy some comments. One thing that I've noticed is that a search with no results now raises a ValueError, rather than giving an empty SelectResult, i.e. one with zero results. There are a few places in BioSimSpace where we perform a search then check that search.nResults() == 0. I'll need to go through and replace these with appropriate try/except blocks.

(The new behaviour makes more sense to me, since it is good to know that a search is empty/invalid immediately.)

@lohedges
Copy link
Member

Bah, it turns out that the behaviour is inconsistent, i.e. it sometimes returns an empty search if the string is valid but there is no result, but sometimes returns a ValueError. For example:

# Search for an unknown element.
system.search("element Z")

ValueError: 'Invalid search query: 'element Z' : SireMol::parse_error: Failed to parse any of the selection 'element Z'. (call sire.error.get_last_error_details() for more info)

# Search for a valid element, using an invalid property map.
system.search("element H", property_map={"element" : "cheese"})

ValueError: 'Invalid search query: 'element H' : 'Nothing matched the search.'

# Now the same thing on the first molecule in the system.

# Search for an invalid element.
molecule.search("element Z")
# Same error as before.
ValueError: 'Invalid search query: 'element Z' (SireMol::parse_error: Failed to parse any of the selection 'element Z'. (call sire.error.get_last_error_details() for more info))

# Search for a valid element, using an invalid property map.
molecule.search("element H", property_map={"element" : "cheese"})

# No error, rather an empty result is returned.
<BioSimSpace.SearchResult: nResults=0>

I'll try to figure out why this is, since we need the behaviour to be consistent.

@lohedges
Copy link
Member

Ah, this is because BioSimSpace._SireWrappers.Molecule.search() returns an empty result when the search fails, whereas the other wrappers raise the exception:

        try:
            # Query the Sire molecule.
            search_result = query(self._sire_object, property_map)
        except KeyError:
            # Nothing matched - return an empty result
            search_result = _SireMol.SelectorMol()

I'll change this for consistency.

@lohedges
Copy link
Member

lohedges commented Sep 23, 2022

Okay, I've needed to make quite a few edits to handle the new search functionality. However, it looks like the existing searches used to find atoms when applying different types of restraint (using system.getRestraintAtoms()) are no longer valid. I'll need to work out why this is, then adapt the search strings to whatever the equivalent is for Sire 2023.0.0. (I'll also add some tests so that validate the various restraint search terms that we allow.)

(Definitely highlighting a lack of unit tests in certain areas where functionality was blasted out for a workshop.)

@lohedges
Copy link
Member

This is now fixed. There were some issues with formatting for combining searches via a logical and. We also no longer need to do searches like atoms in ... since atoms are returned by default.

@lohedges lohedges merged commit 0fc24e0 into devel Sep 26, 2022
@lohedges lohedges deleted the feature-web branch September 26, 2022 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants