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

add magres datatype #17423

Conversation

Neonbluestoplight
Copy link
Contributor

Added support for Ab initio Nuclear Magnetic Resonance file format (Magres). These are needed for Nuclear Magnetic Resonance (materials science) use cases. Also moved ASE metadata output functionality to its own class (AtomicStructFile) for easier addition of future datatypes that make use of ASE for metadata.

See:
https://www.ccpnc.ac.uk/docs/magres

cc @patrick-austin

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

Neonbluestoplight and others added 8 commits January 31, 2024 16:59
Transfered the metadata parsing using the ASE IO library to an abstract
class "AtomicStructFile"
Earlier parent class was Text but was changed to GenericMol file for
XYZ and Extended XYZ files
Also removed set_peek() method from extxyz due to functionality
being present in AtomicStructFile abstract class.
Changed DatasetProtocol file_name attribute to be
compatible with galaxy v23.1
@github-actions github-actions bot added this to the 23.2 milestone Feb 5, 2024
@martenson
Copy link
Member

@Neonbluestoplight Hi, thanks for the contribution. New datatypes should generally target the dev version of Galaxy I think -- so could you please retarget your PR?

@martenson martenson changed the title Release 23.1 magres add magres datatype Feb 5, 2024
@bgruening
Copy link
Member

@martenson from https://github.com/galaxyproject/galaxy/blob/dev/doc/source/project/organization.rst#handling-pull-requests

Pull requests modifying frozen and tagged release branches should be restricted to bug fixes. As an exception, pull requests which only add new datatypes can target a frozen branch or the latest tagged release branch.

The question is if this is too much of a refactoring. @Neonbluestoplight do you need it in 23.1?

@@ -771,6 +771,7 @@
<datatype extension="cif" type="galaxy.datatypes.molecules:CIF" display_in_upload="true"/>
<datatype extension="xyz" type="galaxy.datatypes.molecules:XYZ" display_in_upload="true"/>
<datatype extension="extxyz" type="galaxy.datatypes.molecules:ExtendedXYZ" display_in_upload="true"/>
<datatype extension="magres" type="galaxy.datatypes.molecules:Magres" display_in_upload="true"/>
Copy link
Member

Choose a reason for hiding this comment

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

If you like please add a description=" and description_url="

@patrick-austin
Copy link
Contributor

Hi both - I advised @Neonbluestoplight to target 23.1 based on the guidelines which Bjoern linked (we've always targetted the latest tagged release branch in the past when adding Datatypes). We didn't notice the repeated code that could be refactored until later on.

@Neonbluestoplight do you need it in 23.1?

We don't need this desperately, as it's for an emerging use case we're looking to support in the future. Is there a anticipated release date for 23.2?

If you like please add a description=" and description_url="

Didn't know these were possible xml tags, we can try adding this for all our datatypes as well (since I'm pretty sure they don't have them either).

@martenson
Copy link
Member

@patrick-austin 23.2 went out on the 30th of January. I think that would be a better target, but dev is also alright, as 24.0 will likely go out in two months or so. If there is no current need for having this in 23.1 I would recommend not merging it there.

@jdavcs
Copy link
Member

jdavcs commented Feb 7, 2024

I'm sorry, but I'm afraid it's too late for 23.2. This can make it into 24.0. As per @martenson, you'll need to retarget this PR for dev.

@jdavcs jdavcs modified the milestones: 23.2, 24.0 Feb 7, 2024
@patrick-austin
Copy link
Contributor

Ah that's the source of confusion then - we were looking at https://github.com/galaxyproject/galaxy/releases where the latest release is 23.1.4 back on January 6th (and I only just got an email advertising the features of 23.2 last night via the mailing list). I can see that 23.2 does appear in https://github.com/galaxyproject/galaxy/tags though - I suppose tags are still in use but GitHub "releases" aren't accurate (anymore)?

@jdavcs
Copy link
Member

jdavcs commented Feb 7, 2024

@patrick-austin @Neonbluestoplight Correction: it is not too late since this is a datatype addition. Sorry for the confusion - my mistake! So yes, please target the release_23.2 branch and we'll merge it.
The releases vs tags pages are indeed confusing, we'll fix that (both should be accurate).

@nsoranzo
Copy link
Member

I guess this can be closed in favour of #17453 ?

@Neonbluestoplight
Copy link
Contributor Author

Neonbluestoplight commented Feb 12, 2024

Yes, I was about to close this right now. Thank you

@martenson
Copy link
Member

I added the latest release to https://github.com/galaxyproject/galaxy/releases -- sorry for the confusion @Neonbluestoplight

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants