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.

Sign up for GitHub

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

Jump to bottom

Geomx model addition l1 l2 l3 #245

Merged
merged 32 commits into from
Jul 31, 2023
Merged

Geomx model addition l1 l2 l3 #245

merged 32 commits into from
Jul 31, 2023

Conversation

PozhidayevaDarya
Copy link
Contributor

@PozhidayevaDarya PozhidayevaDarya commented Jun 27, 2023 •
edited by adamjtaylor
Loading

Changes needed for attributes and components related to the implementation of GeoMx Spatial Tx metadata template.

Closes #246

clarisse-lau
clarisse-lau reviewed Jun 27, 2023
View reviewed changes
HTAN.model.csv Outdated Show resolved Hide resolved
@clarisse-lau
Copy link
Contributor

Should we consider calling the variables Synapse ID of GeoMx PKC file, etc to avoid confusion?
The current attribute is 'Path to' but description calls for Synapse ID

@PozhidayevaDarya PozhidayevaDarya mentioned this pull request Jun 27, 2023
Closed
9 tasks
adamjtaylor
adamjtaylor requested changes Jun 28, 2023
View reviewed changes
Copy link
Contributor

@adamjtaylor adamjtaylor left a comment

Choose a reason for hiding this comment

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

Thanks, @PozhidayevaDarya! I've suggested some changes and will re-review once implemented. Happy to discuss any of these.

HTAN.model.csv Outdated Show resolved Hide resolved
HTAN.model.csv Outdated Show resolved Hide resolved
HTAN.model.csv Outdated Show resolved Hide resolved
HTAN.model.csv Outdated Show resolved Hide resolved
HTAN.model.csv Outdated Show resolved Hide resolved
HTAN.model.csv Outdated Show resolved Hide resolved
HTAN.model.csv Outdated Show resolved Hide resolved
HTAN.model.csv Outdated Show resolved Hide resolved
@adamjtaylor
Copy link
Contributor

adamjtaylor commented Jun 28, 2023 •
edited
Loading

@PozhidayevaDarya, we might be out of sync between the CSV and the JSON-LD due to multiple actions running simultaneously (Something we are looking to fix in #224). I am re-running the last action and see if that gets us up to date and will comment here. Rerunning the action still caused an error. I'll see if I can fix this week.

Updated: Fixed by converting locally

@adamjtaylor adamjtaylor mentioned this pull request Jun 28, 2023
Closed
@aclayton555 aclayton555 added the effort-mid This one needs your brain label Jun 28, 2023
@adamjtaylor
Copy link
Contributor

I was able to convert the CSV to JSON-LD locally and this allowed the checks to complete.

adamjtaylor
adamjtaylor requested changes Jun 29, 2023
View reviewed changes
Copy link
Contributor

@adamjtaylor adamjtaylor left a comment

Choose a reason for hiding this comment

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

Just one minor comment

HTAN.model.csv Show resolved Hide resolved
@adamjtaylor
Copy link
Contributor

Looks good to me. @elv-sb can you take a quick pass also. If happy please approve and merge and we can deploy to staging to allow @PozhidayevaDarya to do some testing

@adamjtaylor adamjtaylor requested a review from elv-sb June 30, 2023 12:51
@PozhidayevaDarya
Copy link
Contributor Author

Thank you!

@adamjtaylor
Copy link
Contributor

@elv-sb can you take a look before the mid-sprint review today and then hopefully we can get up to staging today.

@adamjtaylor adamjtaylor self-requested a review July 10, 2023 18:11
@adamjtaylor
Copy link
Contributor

Review re-requested by myself as @PozhidayevaDarya has a few more commits to make.

adamjtaylor
adamjtaylor approved these changes Jul 28, 2023
View reviewed changes
Copy link
Contributor

@adamjtaylor adamjtaylor left a comment

Choose a reason for hiding this comment

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

LGTM 👻

@PozhidayevaDarya
Copy link
Contributor Author

YAY!! tysmm

@PozhidayevaDarya
Remove Parent File ID
6562cc6 
Parent File ID not applicable because count matrix not derived from images.
@adamjtaylor
Copy link
Contributor

@PozhidayevaDarya that last commit failed due to ['HTAN Parent Data File ID is missing from DependsOn for attribute NanoString GeoMx DSP Spatial Transcriptomics Level 3']. This currently is required for Levels 2-4. Let me know if you need an exception to this test.

@PozhidayevaDarya
Copy link
Contributor Author

@adamjtaylor Yes this is intentional and should be an exception. Thanks!

@adamjtaylor
Copy link
Contributor

@PozhidayevaDarya I was able to fix the test, so this is good to go

@adamjtaylor
Copy link
Contributor

@clarisse-lau your review is still open. Can you re-approve?

@adamjtaylor
Copy link
Contributor

Super. Updating from main to avoid any conflicts and then merging.

@adamjtaylor adamjtaylor merged commit 70eecf8 into main Jul 31, 2023
4 checks passed
@adamjtaylor adamjtaylor deleted the geomx-model-addition-l1-l2-l3 branch July 31, 2023 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamjtaylor adamjtaylor adamjtaylor approved these changes

@clarisse-lau clarisse-lau clarisse-lau approved these changes

@elv-sb elv-sb elv-sb approved these changes

Assignees
No one assigned
Labels
effort-mid This one needs your brain
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Geomx L1-L3 Data Model Update
5 participants
@PozhidayevaDarya @clarisse-lau @adamjtaylor @elv-sb @aclayton555