Skip to content

Conversation

@rowlesmr
Copy link
Collaborator

@rowlesmr rowlesmr commented Jul 1, 2025

from COMCIFS/MultiBlock_Dictionary#31 (comment)

_pd_diffractogram will want to point to _diffrn_scan.scan_id and _diffrn_scan_frame.frame_id if it is derived from a 2D measurement and there is more than one scan + frame.

Starting the process to allow a diffractogram to link to the specifc imgCIF frame and/or scan from which it was originally measured

@vaitkus
Copy link
Collaborator

vaitkus commented Jul 1, 2025

@rowlesmr, an unsolicited piece of advice. If you want to successfully run the DDLm checks, you will have to also update the .github/workflows/main.yml file to explicitly download the imgCIF file. However, even then I am unsure if the checks will pass as the automatic DDL2 -> DDLm conversion of the dictionary might have left some inconsistencies in the imgCIF dictionary.

Nevertheless, it might be useful to an extent.

@rowlesmr
Copy link
Collaborator Author

rowlesmr commented Jul 1, 2025

Ta a lot! The automation side of things is where I have little idea what is going on

@rowlesmr
Copy link
Collaborator Author

rowlesmr commented Jul 1, 2025

whelp. That is beyond my understanding.

@vaitkus
Copy link
Collaborator

vaitkus commented Jul 1, 2025

whelp. That is beyond my understanding.

You did well, but as I initially guessed, there are still some discrepancies that might need to be addressed upstream in the imgCIF dictionary repository. Also, imgCIF seems to import a fixed revision of the coreCIF dictionary (version 3.2.0) which would again require modifying the workflow (my programs do not yet attempt to download dictionaries from the specified URL).

This was quite interesting, though. Feel free to comment out the DDLm checks for now if they are too annoying.

@rowlesmr
Copy link
Collaborator Author

rowlesmr commented Jul 2, 2025

But in any case, it appears that the errors/warnings are all with imgCIF, at least at this stage.

From what I can tell reading the referenced core CIF version, it looks like most of the errors/warnings are the fault of imgCIF?

Ah. But also, this was done against COMCIF/img_cif, where it probably should have been done against yayahjb/cbf_imgcif_dictionary/ddlm/img_cif. A quick read through there shows much the same issues. But considering that the ddlm version is a translation of the ddl2, I know not if it is an error in translation, or in the original.

@jamesrhester
Copy link
Contributor

But in any case, it appears that the errors/warnings are all with imgCIF, at least at this stage.

From what I can tell reading the referenced core CIF version, it looks like most of the errors/warnings are the fault of imgCIF?

Ah. But also, this was done against COMCIF/img_cif, where it probably should have been done against yayahjb/cbf_imgcif_dictionary/ddlm/img_cif. A quick read through there shows much the same issues. But considering that the ddlm version is a translation of the ddl2, I know not if it is an error in translation, or in the original.

The imgCIF dictionary is a moving target in terms of which repository it will end up in. It is currently being transferred to the DIALS repository, but the DDLm version, as far as I know, should essentially be the same as the one in the yayahjb repo that you reference.

For the purposes of making progress within PD, I think we should ignore imgCIF-related errors, and instead raise an issue in the COMCIFS/imgCIF repository as a way to keep track of imgCIF-PD integration problems.

I have had a very slow time of getting any changes in the imgCIF dictionary approved, but I'm hoping that will improve with the new home.

@jamesrhester
Copy link
Contributor

Most/all of the errors are just translation issues, looking at them. I have raised an issue.

@rowlesmr
Copy link
Collaborator Author

rowlesmr commented Jul 3, 2025

For the purposes of making progress within PD, I think we should ignore imgCIF-related errors, and instead raise an issue in the COMCIFS/imgCIF repository as a way to keep track of imgCIF-PD integration problems.

I can get onboard with that.

@jamesrhester
Copy link
Contributor

I don't think we can really assert that only a single frame may contribute to a diffractogram, as these additions imply. Here we often collect frames every few minutes but then smoosh them together for the final diffractogram. Perhaps what we want is to stipulate that a single 'scan' contributes to a single diffractogram. That means a typical single-exposure diffractogram would be a one-frame scan. The _pd_diffractogram.scan_id points to the scan, and the contributing frames are determined from the imgCIF data.

So we don't need the _pd_diffractogram.frame_id pointer.

@rowlesmr
Copy link
Collaborator Author

rowlesmr commented Jul 4, 2025

For the moment, I've removed the imgCIF import from the github workflow. I figured we know where the issues come from, and it's probably better having three two issues we can ignore for the moment, than dozens. Also, I don't know how (or even if it's possible) to exclude a specific dictionary from checks.

The only changes in the workflow yaml file are EOL whitespace removal.

@rowlesmr rowlesmr marked this pull request as ready for review July 5, 2025 05:19
@jamesrhester jamesrhester merged commit 563bf99 into COMCIFS:master Jul 9, 2025
@rowlesmr rowlesmr deleted the add-imgCIF-specific-to-diffractogram branch July 11, 2025 13:37
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.

3 participants