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

Move around cell 2 #358

Merged
merged 10 commits into from
Mar 16, 2023
Merged

Conversation

jamesrhester
Copy link
Contributor

This replaces #299, dealing with issue #294 . Please refer to the discussion in those places for information. This PR provides nothing new compared to #299 , it just accounts for the updates to master.

Because of the reordering of the definitions, the usual Github diff looks very messy. The semantic changes are in commit 339aaa7 , so probably best to look at that to understand the changes. The commit after that only changes the order and pretty-prints.

James.Hester added 2 commits March 2, 2023 12:32
    The CELL category parent is now DIFFRN.
    pressure, temperature items in CELL_MEASUREMENT deprecated
    in favour of similar items in DIFFRN.

    Pointers to _diffrn.id added to CELL etc. to allow multiple
    cells to be presented where multiple diffraction
    conditions are used.

    The `CELL_MEASUREMENT` category now has two `_diffrn.id` pointers, one to
    specify the data collection to which the cell measurement applies, and
    one to the conditions under which the cell was measured (`.condition_id`).
    Normally these coincide and the latter is not necessary, but legacy point
    detector experiments can sometimes involve measuring the cell under
    differing conditions prior to data collection.
@jamesrhester
Copy link
Contributor Author

If @vaitkus and other commenters are happy with the current version, I'd like to merge so that we can proceed with a proper release of core CIF. Please advise.

@vaitkus
Copy link
Collaborator

vaitkus commented Mar 12, 2023

@jamesrhester thank you for the work and sorry for not responding sooner. As discussed, I added two examples files that illustrate the new approach to recording cell measurement details based on the discussions in #299. Please check if they are ok. The single block example files currently invents a few data names (e.g. _diffrn_radiation.diffrn_id), places several Set category items in a loop (_diffrn.ambient_temperature) and groups a few items items from different categories in a single loop (e.g. _diffrn_radiation_wavelength.value), but I guess that some of these restrictions can be lifted in future commits.

Once you are happy with these examples, it would be nice if some other stakeholders could review them as well. I will ask some of my colleagues from the COD to take a look, but maybe there are some other database/IUCr archive people who would be affected by this that you are aware of?

@jamesrhester
Copy link
Contributor Author

I have updated the examples to clarify a few things.

Once you are happy with these examples, it would be nice if some other stakeholders could review them as well. I will ask some of my colleagues from the COD to take a look, but maybe there are some other database/IUCr archive people who would be affected by this that you are aware of?

This change is a logical extension of the multi-block approach that was discussed at length. What I propose doing is, as part of the release process, providing a proper description of all the changes (perhaps as a wiki page) and inviting everybody on COMCIFS/Core-DMG/cif-developers to review what has been written about those changes. Reviewers would have a few weeks to comment.

@vaitkus
Copy link
Collaborator

vaitkus commented Mar 15, 2023

Ok, so from your updated comment in the cell-measurement-merged-block.cif example file I understood that I misinterpreted the general approach and went slightly overboard by trying to force the merged data block representation into the multi-block paradigm. Since the CELL_MEASUREMENT items are only deprecated and not outright removed, the COD can still keep using them to represent old archival files and will eventually adapt to process the multi-block files. If you also see no additional value from the custom audit schema introduced in cell-measurement-merged-block.cif, I suggest that we remove this file before the merge as to not confuse the readers with needless extra details.

On a related note, I added an example that uses the deprecated items and tried to highlight the benefits of the multi-block approach.

Finally, I will probably add the superfluous _definition_replaced.id items just to silence the validator for now. I promise to remove them once the validator is patched.

If you are good with these changes, I am happy to merge.

@jamesrhester
Copy link
Contributor Author

I'm happy with those changes, so I think we are ready to merge?

The file is no longer relevant as an example.
@vaitkus
Copy link
Collaborator

vaitkus commented Mar 16, 2023

Great. @jamesrhester, should I merge it or will you?

@jamesrhester jamesrhester merged commit 9aa3e2f into COMCIFS:master Mar 16, 2023
@jamesrhester
Copy link
Contributor Author

Thanks everybody for your contributions to this very complicated PR!

@jamesrhester jamesrhester deleted the move_around_cell_2 branch June 28, 2023 05:15
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.

2 participants