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 AnalyticalSample class from schema #77

Merged
merged 3 commits into from
Feb 2, 2024

Conversation

brynnz22
Copy link
Collaborator

This PR removes the AnalyticalSample class from the schema.

Addresses issue microbiomedata#1681

Copy link
Collaborator

@mslarae13 mslarae13 left a comment

Choose a reason for hiding this comment

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

@brynnz22 I did a quick search for AnalyticalSample & it looks like this class is mentioned in

notebooks/in-repo-anaggregateds-report.ipynb

&
assets/TermsUpdated_organicmatterextraction/TermsUpdated_organicmatterextraction.tsv

I was checking for example files. There are none, so I think this is good to be merged. Just wanted to note the above in the event it mattered for migrtation.

@brynnz22
Copy link
Collaborator Author

brynnz22 commented Feb 1, 2024

@turbomam I removed the AnalyticalSample rows from the tsv @mslarae13 mentioned, though I am not sure what that tsv is for. As for the notebook: notebooks/in-repo-anaggregateds-report.ipynb, AnalyticalSample is mentioned as an output of a print statement, so this notebook just needs to be rerun and it shouldn't have AnalyticalSample since it is removed from the schema. I could not figure out how to run this notebook in a poetry environment, the import statements don't compute. At least I think this is the problem. Since this is just a print statement, do you want me to a) rerun the notebook (if so, can you tell me how I do this with poetry as the dependency management tool?, or b) not worry about it because technically no code is changing?

@turbomam
Copy link
Member

turbomam commented Feb 2, 2024

Thanks for doing that research @mslarae13 and @brynnz22

I will run the notebook and let you know if I had to do anything tricky. I agree that this is a low risk situation, since the string AnalyticalSample only appears as a cached output in the notebook, and not part of any code lines.

As for assets/TermsUpdated_organicmatterextraction/TermsUpdated_organicmatterextraction.tsv: I believe that came from very old attempts that @mslarae13 and I made for creating something like DataHarmonizer solely within Excel or GoogleSheets. I would prefer to totally remove it or leave it as is. It is not mentioned anywhere else in the repo.

@brynnz22 brynnz22 merged commit c9277a9 into main Feb 2, 2024
2 checks passed
@brynnz22 brynnz22 deleted the 1681-remove-AnalyticalSample-class branch February 2, 2024 17:03
@brynnz22 brynnz22 restored the 1681-remove-AnalyticalSample-class branch February 2, 2024 17:09
@brynnz22 brynnz22 changed the title remove AnalyticalSample class from schema remove AnalyticalSample class from schema Mar 27, 2024
@mslarae13
Copy link
Collaborator

No migration needed IF AnalyticalSample was never used

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