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

(Do not merge until validation recalculation) Use mmCIF validation reports instead of XML #16

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

Conversation

aliciaaevans
Copy link
Contributor

@aliciaaevans aliciaaevans commented Jun 27, 2023

Once validation recalculation is done, every entry will have an mmCIF validation report. ThetoCifWrapper marshal helper and internal validation dictionary will no longer be needed. Instead, the CIF can be merged with the structure's mmCIF.

@aliciaaevans aliciaaevans changed the title (Do not merge until validation remediation) Use mmCIF validation reports instead of XML (Do not merge until validation recalculation) Use mmCIF validation reports instead of XML Jun 27, 2023
@aliciaaevans
Copy link
Contributor Author

aliciaaevans commented Jun 27, 2023

Note: the testLocalRepoUtils tests won't pass until after the recalculation because it's expecting a certain number of categories, including vrpt, to be found. Also, the validation cif files need to be added to the mock-data repo.

@brindakv brindakv requested a review from piehld December 17, 2024 17:16
Copy link
Collaborator

@piehld piehld left a comment

Choose a reason for hiding this comment

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

Thanks @brindakv. Overall looks good to me. Just had a couple code-cleanliness comments.

Two other general comments:

  • Remember to update HISTORY.txt and the __init__.py file to increment the version number
  • Looks like there is an error in the Azure pipeline. Doesn't seem to be related to your changes, but will be necessary to address before merging to master.

# kwD = HashableDict({"marshalHelper": vrd.toCif})
kwD = HashableDict({"marshalHelper": toCifWrapper})
oL.append(HashableDict({"locator": mergeLocator, "fmt": "xml", "kwargs": kwD}))
kwD = HashableDict({})
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is already defined earlier in this loop (see line 239), is there really a need to redefine it?

@@ -574,8 +573,8 @@ def __getEntryUriList(self, idCodeList=None, mergeContentTypes=None):
if mergeContentTypes and "vrpt" in mergeContentTypes:
# if self.__chP.hasEntryContentType(tId, "validation_report"):
if self.__chP.hasValidationReportData(tId):
kwD = HashableDict({"marshalHelper": toCifWrapper})
locObj.append(HashableDict({"locator": self.__getLocatorRemote("validation_report", tId), "fmt": "xml", "kwargs": kwD}))
kwD = HashableDict({})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same question here (but in reference to line 571).

Copy link
Collaborator

@piehld piehld Dec 17, 2024

Choose a reason for hiding this comment

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

And actually, for both questions, now that you're not changing kwD between consecutive loop iterations, could this just be defined once outside the scope of the loop?

@@ -742,8 +741,8 @@ def _entryLocatorObjWithMergeWorker(self, dataList, procName, optionsD, workingD
idCode = fn[:4] if fn and len(fn) >= 8 else None
mergeLocator = self.__getLocator(mergeContentType, idCode, checkExists=True) if idCode else None
if mergeLocator:
kwD = HashableDict({"marshalHelper": toCifWrapper})
oL.append(HashableDict({"locator": mergeLocator, "fmt": "xml", "kwargs": kwD}))
kwD = HashableDict({})
Copy link
Collaborator

Choose a reason for hiding this comment

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

And here (w.r.t. line 738)

@brindakv
Copy link
Contributor

Thanks @piehld. I have addressed all the comments except the azure pipeline error. I think this will need mock-data to be updated first.

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