-
Notifications
You must be signed in to change notification settings - Fork 39
Clean Amin1.0 branch from master #839
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
Conversation
|
Hi @tristanpwdennis , @ahernank , @jonbrenas Noting linting check failures here: Noting that Looking at the existing code for # Convenience variables.
data["country_location"] = data["country"] + " - " + data["location"]....and line 458 is similar: # Convenience variables.
data["country_location"] = data["country"] + " - " + data["location"]I recognise this problem, which has already been addressed in the pending PR #833 The solution appears to be to ensure that the country and location values are strings. For example, in the pending PR mentioned: # Convenience variables.
# Prevent lint error (mypy): Unsupported operand types for + ("Series[Any]" and "str")
data["country_location"] = (
data["country"].astype(str) + " - " + data["location"].astype(str)
) |
|
Hi @tristanpwdennis , #833 has been merged now. |
5c03d92 to
62ff9e0
Compare
|
Tests are passing locally but failing here? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #839 +/- ##
==========================================
+ Coverage 90.68% 92.01% +1.33%
==========================================
Files 49 49
Lines 5175 5175
==========================================
+ Hits 4693 4762 +69
+ Misses 482 413 -69 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ahernank
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 so much @tristanpwdennis, super exciting! I've left some minor notes after a light review.
malariagen_data/amin1.py
Outdated
| f"Data filtered to unrestricted use only: {self._unrestricted_use_only}\n" | ||
| f"Data filtered to surveillance use only: {self._surveillance_use_only}\n" |
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 don't think the data-side of these has been sorted. I am conscious part of this is because we are missing accessions for some of the vobs-asia samples. What do you think about commenting these out to avoid confusion around this functionality and we can open an issue separately to fix these for Adir/Amin later on?
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 don't think we need this file here, as this is up in curation.
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.
It seems there is an extra metadata/metadata dir here, including an extra copy of this file & the ones below.
Co-authored-by: Anastasia Hernandez-Koutoucheva <[email protected]>
Co-authored-by: Anastasia Hernandez-Koutoucheva <[email protected]>
Co-authored-by: Anastasia Hernandez-Koutoucheva <[email protected]>
Co-authored-by: Anastasia Hernandez-Koutoucheva <[email protected]>
ahernank
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 @tristanpwdennis, looks good to me, I've added an issue on vobs-asia to address the surveillance flags/accessions separately. Happy to merge and we can do any other fixes post.
See #825