Conversation
There was a problem hiding this comment.
Ok, this is a tough one to review, because I'm lacking the physical understanding to judge whether the relations are wrong indeed. On the face of it, it seems that you are right, though. Therefore, at this point, only a few unsorted comments and suggestions:
- @kara-mela, you were the the only editor of the IUPAC mapping before this PR, so I trust that you know what you're doing when retracting some of the relations. That is why, in principle, I'd be happy to approve despite my lack of understanding in the details,
- I made a suggestion on the change in line 13, though. Reverting that change would fix the merge conflict,
- I'd suggest to merge the latest version of
masterinto your branch before the final review. As I understand.github/workflows/build.ymlthis would fix the failing build (because in the latest version onmaster, it would skip trying to deploy).
| # mapping_set_description: Manually curated alignment of the PaNET ontology with the IUPAC goldbook.,,,,,,,,, | ||
| # Intended to be used for ontological analysis.,,,,,,,,, | ||
| # mapping_set_id: mapping_goldbook_PaNET_v1.0,,,,,,,,, | ||
| # mapping_set_id: mapping_goldbook_PaNET_v2.0.0,,,,,,,,, |
There was a problem hiding this comment.
Why are changing the PaNET version in mapping_set_id here? I can't see how this is related to the intention of your PR. This change is causing the merge conflict with the master branch. I'd rather suggest to leave that line alone, e.g. to revert the change in this line:
| # mapping_set_id: mapping_goldbook_PaNET_v2.0.0,,,,,,,,, | |
| # mapping_set_id: mapping_goldbook_PaNET_v1.0,,,,,,,,, |
|
Thanks a lot for your review, @RKrahl! I guess some of the context is hidden too deep down in the list of subsequent links. Sorry for that. The review of the first version of the IUPAC mapping was performed by two different people in parallel, where the slower review highlighted several opportunities for improvement. These comments can be found in the original merge request. We now try to integrate the listed concerns step by step. One point was, that I assigned some terms wrongly as related to each other, to which I agree. This means that the update is not the opinion of a single person, but backed up by at least two people. Concerning the version of the mapping: Between initially creating the IUPAC mapping and this merge request, we decided to use semantic versioning. Additionally, I considered deleting some lines to be a breaking change. The current version is v1.0.1. When my previous assumptions are correct, I would still go for v2.0.0. Please let me know if I understood something wrong. |
Motivation Several relationships in the mapping between IUPAC and PaNET were wrong and neet to be deleted. Modification The relationships were deleted. Result Improved correctness of the mapping.
3b8c9af to
cf9c6d5
Compare
Motivation
Several relationships in the mapping between IUPAC and PaNET were wrong and neet to be deleted.
Modification
The relationships were deleted.
Result
Improved correctness of the mapping.
Related Issues
Closes #422