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

Remove has_calibration from WorkflowExecution subclasses after MassSpectrometry change sheets #1852

Open
brynnz22 opened this issue Mar 20, 2024 · 8 comments
Assignees

Comments

@brynnz22
Copy link
Contributor

brynnz22 commented Mar 20, 2024

see issue: #1570

In issue linked above, discussed creating a Calibration class. Will need to change name of slot from has_calibration which implies a boolean and we are linking a DataObject.

This will require a migration.

@brynnz22 brynnz22 changed the title Remove has_calibration from WorkflowExecution subclasses after MaterialProcessing ingest Remove has_calibration from WorkflowExecution subclasses after MaterialProcessing ingest and possibly refactor Mar 27, 2024
@brynnz22
Copy link
Contributor Author

brynnz22 commented Apr 5, 2024

After discussion with @kheal and @anastasiyaprymolenna has_calibration is fine the way its modeled on ChromatographicSeparationProcess. Calibration does not need to have its own class. There are no calibration reagents to capture.

@kheal
Copy link
Contributor

kheal commented Apr 17, 2024

See update here: #1918 . After discussing limitations of only capturing the a DataObject with regards to calibration, we will likely make a Calibration class.

@kheal
Copy link
Contributor

kheal commented May 1, 2024

See microbiomedata#133.

After ingestion and removal of has_calibration from WorkflowExecutionActivity

  1. The range of has_calibration on CalibrationInformation class should be limited toCalibrationInformation.
  2. The definitions for CalibrationInformation, has_calibration, calibration_target, CalibrationTargetEnum, internal_calibration, calibration_standard, CalibrationStandardEnum, and calibration_object can move from the workflow_execution.yaml to the a logical location of nmdc.yaml

@aclum
Copy link
Contributor

aclum commented Jun 19, 2024

Is this planned for after the rollout?

@kheal
Copy link
Contributor

kheal commented Jun 19, 2024

@aclum . As part of the rollout, @brynnz22 and I will need to make change sheets for existing GC-MS metabolomics and NOM DataGeneration records to refactor the use of the 'has_calibration' slot. After that we can remove that slot and its contents from the WorkflowExecution class. If we remove that slot before the change sheets, we'll orphan the DataObjects that are linked through that slot.

@brynnz22 brynnz22 changed the title Remove has_calibration from WorkflowExecution subclasses after MaterialProcessing ingest and possibly refactor Remove has_calibration from WorkflowExecution subclasses after MassSpectrometry change sheets Jul 3, 2024
@brynnz22 brynnz22 self-assigned this Jul 3, 2024
@kheal kheal self-assigned this Jul 22, 2024
@kheal
Copy link
Contributor

kheal commented Jul 22, 2024

Note that this is dependent on the completion of microbiomedata/issues#750

@brynnz22
Copy link
Contributor Author

brynnz22 commented Jul 25, 2024

@kheal after thinking about this further, I don't think we should add the has_calibration in the change sheet and write a migrator to remove the slot from the WorkflowExecution. This allows room for a lot of human error. I think we should go with what I suggested last week. Write a migrator that moves it upstream to the MassSpectrometry its on, and then remove. This allows us to do checks and make sure its on the correct instance. Whereas using a change sheet then removing it in a migrator allows for possible errors of mismatching. I can write the migrator for this. I will also do the change sheet. thanks!

@kheal
Copy link
Contributor

kheal commented Jul 25, 2024

@brynnz22 Sounds reasonable. I certainly prefer migrators over changesheets and I agree there are fewer opportunities for errors. I'll update the microbiomedata/issues#750 with this new plan. Will you make a new issue (or change this issues' name/description?) to describe the migrator? I think I understand it, but I think you'll capture it more thoroughly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Post Berk Roll Out
Development

No branches or pull requests

3 participants