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 horizon_method slot #157

Conversation

bmeluch
Copy link
Contributor

@bmeluch bmeluch commented Sep 21, 2023

remove horizon_method slot from waterinterface

MAM 2023-09-27: added a link to the issue here:

remove horizon_method slot from waterinterface
@bmeluch bmeluch linked an issue Sep 21, 2023 that may be closed by this pull request
@bmeluch bmeluch requested a review from pkalita-lbl September 21, 2023 21:43
Copy link
Collaborator

@pkalita-lbl pkalita-lbl left a comment

Choose a reason for hiding this comment

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

Hey thanks for taking a crack at this! However the submission schema is, for lack of a better word, weird. Specifically, the file you've changed actually gets built based on other files. The file you want to look at is sheets_and_friends/tsv_in/import_slots_regardless.tsv. In particular this line says "take the horizon_method slot from the nmdc_schema and plop it into the following classes of the submission_schema: AirInterface, BiofilmInterface, etc". So you'd want to remove WaterInterface from that list. Then you can regenerate all the generated files (there will be a lot!) and run tests with make all.

@turbomam
Copy link
Member

turbomam commented Sep 27, 2023

Thanks @bmeluch !

Let's set up a conversation between you and either @pkalita-lbl or me about "what files to commit" Sorry, I haven't set this repo to be especially helpful in that regard!

You PRs will be easiest to review if you only commit the files you have changed. It looks like you are either doing the build process on your own or at least getting ready for it. That's great.

Please don't commit changes to examples/output/ or project/. In fact, it's unlikely that you will need to commit files from any path other than src/data/ or sheets_and_friends/

@turbomam
Copy link
Member

turbomam commented Sep 27, 2023

It also looks like you updated the valid data files. That's great too. Please also add a new invalid example file that shows the illegal use of whatever slots/columns you are removing. @pkalita-lbl , @brynnz22 and I can help you with that too.

@pkalita-lbl
Copy link
Collaborator

a conversation between you and either @pkalita-lbl or me about "what files to commit"

You know that conversation with me will start with "we should have drastically fewer generated files checked in in the first place" 😂

Please also add a new invalid example file that shows the illegal use of whatever slots/columns you are removing

I have mixed feelings about this. I understand what the point would be, but where do we draw the line? It's impractical to test every slot that shouldn't be there.

@turbomam
Copy link
Member

turbomam commented Sep 27, 2023

Thanks @pkalita-lbl. Let me ponder what value I think we're getting out of having the examples/output files. I would like for the repo to include a worked example of data being converted between LinkML JSON or YAML, TSV and DH JSON. But I guess what we are checking in now doesn't really help with that.

I can do the same thing for the project files. I do want to generate OWL for inclusion in the triplestore.

I agree that we shouldn't try to cover every exhaustive invalid case, but I would like to add a new invalid case each time some expressiveness is removed form the schema.

@pkalita-lbl pkalita-lbl merged commit bc5c37e into main Sep 27, 2023
@pkalita-lbl pkalita-lbl deleted the 148-soil-horizon-method-appears-in-submission-portal-for-water-samples branch September 27, 2023 21:20
@bmeluch
Copy link
Contributor Author

bmeluch commented Sep 28, 2023

Thank you so much @pkalita-lbl !
@mslarae13 helpful notes from Patrick about how to edit the submission schema

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.

"soil horizon method" appears in submission portal for water samples
3 participants