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

Deprecate block ids #120

Open
wants to merge 30 commits into
base: master
Choose a base branch
from
Open

Conversation

rowlesmr
Copy link
Collaborator

@rowlesmr rowlesmr commented Apr 7, 2023

Just showing what it might look like.

Starting by removing block_id-related data items from new categories.

This will end by deprecating PD_BLOCK, PD_BLOCK_DIFFRACTOGRAM, and PD_PHASE_BLOCK. (can entire categories be deprecated?, I know individual data items can be...)

Needs #92 and #93 merged before it can go through fully.

@rowlesmr
Copy link
Collaborator Author

rowlesmr commented Apr 7, 2023

Going through just this first bit showed how many block_id data items we don't have; everything with a phase/diffractogram_id needs a phase/diffractogram block id, and we currently don't have them.

@rowlesmr
Copy link
Collaborator Author

rowlesmr commented Jun 8, 2023

Needs #92 and #93 merged before it can go through fully.

This is done now.

See #56 for more discussion.

cif_pow.dic Outdated
The _pd_phase_block.id or _pd_phase.id entry points to the CIF block with
structural parameters for each crystalline phase.
The _pd_phase.id entry points to the CIF block with structural parameters
for each crystalline phase.
Copy link
Contributor

Choose a reason for hiding this comment

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

_pd_phase.id doesn't really "point" to anything, it just identifies a phase.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've changed some words and moved that sentence to the actual _pd_phase.id description.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about "_pd_phase.id can be used to associate a block with a particular crystalline phase."

_description.text
;
**DEPRECATED**
Use _pd_phase.id or _pd_diffractogram.id, as necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

_pd_block.id is not really replaced by those other data names. _pd_block.id identifies a data block, they identify phases and diffractograms. If anything, it is replaced by _audit.block_code.

Copy link
Collaborator Author

@rowlesmr rowlesmr Jun 15, 2023

Choose a reason for hiding this comment

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

Strictly, yes, but, the data items that were linked to _pd_block.id were only ever used to identify phases or diffractograms. I can add _audit.block_code as an option in the replacement list.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, it might still confuse some people thinking that a _pd_block.id identifies a block, whereas the other ids identify something completely different. We might need to expand the explanation.

@jamesrhester
Copy link
Contributor

Regarding some of those deprecations, as we haven't had a formal release of the dictionary containing some of these items it would be OK to simply remove them, I think.

@rowlesmr
Copy link
Collaborator Author

rowlesmr commented Jun 15, 2023

I did just plain up remove a bunch. AFAIK the ones that are deprecated existed previously in one form or another.

The block_id-related data items that are deprecated are

  • _pd_block.id
  • _pd_block_diffractogram.id
  • _pd_calib_std.external_block_id
  • _pd_phase_block.id

@rowlesmr
Copy link
Collaborator Author

I believe that if this gets merged, we should consider going to v3.0.0 -- it's a pretty big change in how pdCIF files talk between blocks. phse_id and diffractogram_id, and no block_id.

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