-
Notifications
You must be signed in to change notification settings - Fork 3
RoHub Integration #40
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
base: main
Are you sure you want to change the base?
Conversation
joergfunger
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot, nice work. There are a couple of slight changes that are in particular related to distinguishing between what is specific for one benchmark, and what is applicable to all. The latter should be moved one level up into common.
| env: | ||
| CACHE_NUMBER: 1 # increase to reset cache manually | ||
| SNAKEMAKE_PROVENANCE_FILE: metadata4ing_provenance | ||
| PROVENANACE_FILE_NAME: element_size_vs_max_mises_stress.pdf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spelling error, and the name is misleading, since that is not completely the provenance, but rather the actual result of the simulation, though I have to admit that this is part of the information that is tracked in the provenance file.
| path: ./artifact_files | ||
|
|
||
| - name: Unzip metadata4ing_provenance.zip | ||
| - name: Unzip Snakemake Provevnance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spelling error
| --output_file $PROVENANACE_FILE_NAME | ||
| - name: Upload PDF plot as artifact | ||
| - name: Upload provevance file as artifact |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spelling
| with: | ||
| name: element-size-vs-stress-plot | ||
| path: element_size_vs_stress.pdf | ||
| path: ${{ env.PROVENANACE_FILE_NAME }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spelling
| - name: Upload RoCrate Zip file onto RoHub | ||
| shell: bash -l {0} | ||
| run: | | ||
| python benchmarks/linear-elastic-plate-with-hole/upload_provenance.py \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is also not specific to plate with a hole, so move that also one level up to common
| - name: Validate Provenance | ||
| shell: bash -l {0} | ||
| run: | | ||
| python benchmarks/linear-elastic-plate-with-hole/validate_provenance.py \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not specific to plate with a hole. I would suggest moving it one level up to common.
| from rdflib import Graph | ||
| import matplotlib.pyplot as plt | ||
| from collections import defaultdict | ||
| from provenance import ProvenanceAnalyzer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest decomposing the file into parts that are general (for any benchmark) and parts that are specific to plate with a hole and move the general parts to common.
| @@ -0,0 +1,19 @@ | |||
| tool,radius,length,load,element_size,element_order,element_degree,quadrature-rule,quadrature-degree,young-modulus,poisson-ratio,max_von_mises_stress_nodes,max_von_mises_stress_gauss_points | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we split that into one file per tool?
| @@ -0,0 +1,39 @@ | |||
| import argparse | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is also independent of the benchmark, move to common
| - pip | ||
| - pip: | ||
| - "git+https://github.com/izus-fokus/snakemake-report-plugin-metadata4ing@v1.0.0#egg=snakemake-report-plugin-metadata4ing" | ||
| - "git+https://github.com/izus-fokus/snakemake-report-plugin-metadata4ing@v1.2.0#egg=snakemake-report-plugin-metadata4ing" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the versioning scheme that you use, I personally prefer this with
- MAJOR version when you make incompatible API changes
- MINOR version when you add functionality in a backward compatible manner
- PATCH version when you make backward compatible bug fixes
I’ve updated the branch with the latest changes, including:
ro-crate-metadata.json, ensuring the provenance source remains consistent even after uploading the RO to RoHub.Please note that this PR may introduce conflicts in the
run-benchmark.ymlfile. I have renamed the 'tests' job to 'run-simulation'. Additionally, two GitHub secrets have been added to the repository to enable the RoHub integration.