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

Pipeline: Namespace management update. #29

Open
wants to merge 6 commits into
base: pipeline
Choose a base branch
from

Conversation

Raphael-Gazzotti
Copy link

@Raphael-Gazzotti Raphael-Gazzotti commented Sep 25, 2024

versions files should be corrected with future core, computation and neuroimaging commits.

update_commits.py is updated but not tested.

Raphael-Gazzotti and others added 2 commits September 3, 2024 15:47
…be corrected with core and computation commit.

update_commits.py is updated but not tested.
@Raphael-Gazzotti Raphael-Gazzotti changed the title Pipeline Pipeline: Namespace management update. Sep 25, 2024
@lzehl
Copy link
Member

lzehl commented Sep 30, 2024

@Raphael-Gazzotti sorry for causing the conflict here by merging #28 . can you please update?

raphaelgazzotti and others added 2 commits September 30, 2024 15:14
…be corrected with core, computation, controlledTerms last PRs.

update_commits.py is updated but not tested.
@lzehl lzehl self-requested a review October 7, 2024 11:43
Copy link
Member

@lzehl lzehl left a comment

Choose a reason for hiding this comment

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

I think it's okay, but I had to go through it on my phone. @apdavison @olinux please double check and verify

Copy link
Member

@apdavison apdavison left a comment

Choose a reason for hiding this comment

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

Functionally this seems reasonable, although I haven't run it to check.

I've made a few suggestions to make the code easier to understand.

if version == "latest" or float(version[1:]) >= 4:
if TEMPLATE_PROPERTY_LINKED_TYPES in schema_payload["properties"][p]:
schema_payload["properties"][p][TEMPLATE_PROPERTY_LINKED_TYPES] = [
schema.namespaces['types'] + _type.split(":")[-1].split("/")[-1] if (_type.__contains__(
Copy link
Member

Choose a reason for hiding this comment

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

This list comprehension is very hard to understand, and reoccurs a few lines down for embedded types. I suggest breaking this out into a function, and perhaps assigning _type.split(":")[-1].split("/")[-1] to a variable with a meaningful name; at a minimum more comments are needed.

Also, _type.__contains__(x) == True should be written as x in _type.

Copy link
Author

Choose a reason for hiding this comment

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

I've simplified the logic and removed unnecessary code, it should be better now.

openMINDS_pipeline/vocab.py Outdated Show resolved Hide resolved
openMINDS_pipeline/vocab.py Outdated Show resolved Hide resolved
openMINDS_pipeline/vocab.py Outdated Show resolved Hide resolved
Comment on lines 74 to 78
# Only SANDS is in uppercase in schema_group
if s.schema_group == "SANDS":
result[c].append('sands:' + s.type)
else:
result[c].append(s.schema_group + ':' + s.type)

Choose a reason for hiding this comment

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

Could this be changed to
result[c].append(s.schema_group.lower() + ':' + s.type)

Copy link
Author

Choose a reason for hiding this comment

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

It can't be applied here, because of the prefixes specimenPrep and controlledTerms, but I can do a replace instead of a condition.

Choose a reason for hiding this comment

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

Ok, then maybe it is clearer to leave as is

@Raphael-Gazzotti
Copy link
Author

PR#6 in neuroimaging should be accepted before this one and I will update the versions-dev.json file accordingly.

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.

4 participants