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

Fix issue #84 #111

Merged
merged 3 commits into from Jun 21, 2023
Merged

Fix issue #84 #111

merged 3 commits into from Jun 21, 2023

Conversation

lohedges
Copy link
Contributor

This PR closes #84. We now handle missing values in AMBER output files by extending the regex to parse records such as PRESS =********. In all cases, any missing values are interpreted as None. I've modified one of the files used in the AMBER parsing tests to contain a missing value in order to confirm that things work as expected. Currently this code is only applied to the Exscientia Sandpit, where the records are being used. (They are saved to disk after a run.) I'll move this into the core code when I port over the AMBER FEP code.

I've also updated the plotting code to handle None values in time-series data with units. When None is present we filter the both the x and y data (and any associated errors) to remove data corresponding to the None indices in both sets, i.e. if None is present in x, then we remove this value from x and the same index from y. I've not updated the logic for raw float values since this isn't used internally by BioSimSpace, nor the code for plotting contours for 3D data.

I've also removed some trailing whitespace that was introduced in PR #107.

  • I confirm that I have merged the latest version of devel into this branch before issuing this pull request (e.g. by running git pull origin devel): [y]
  • I confirm that I have permission to release this code under the GPL3 license: [y]

Suggested reviewers:

@chryswoods

@lohedges lohedges added bug Something isn't working exscientia Related to work with Exscientia labels Jun 21, 2023
@lohedges lohedges added this to the 2023.3.0 milestone Jun 21, 2023
@lohedges lohedges requested a review from chryswoods June 21, 2023 14:02
@lohedges lohedges temporarily deployed to biosimspace-build June 21, 2023 14:02 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build June 21, 2023 14:02 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build June 21, 2023 14:02 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build June 21, 2023 14:02 — with GitHub Actions Inactive
@lohedges lohedges temporarily deployed to biosimspace-build June 21, 2023 14:02 — with GitHub Actions Inactive
chryswoods
chryswoods previously approved these changes Jun 21, 2023
Copy link
Contributor

@chryswoods chryswoods left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great.

@lohedges lohedges merged commit 82d8938 into devel Jun 21, 2023
@lohedges lohedges deleted the fix_84 branch June 21, 2023 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working exscientia Related to work with Exscientia
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] AMBER Process getPressure would not recognise
2 participants