-
Notifications
You must be signed in to change notification settings - Fork 4
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
Refactor mbon stats #81
base: main
Are you sure you want to change the base?
Conversation
4593d43
to
6721f42
Compare
@MathewBiddle we need to merge #80 first and I'd like to ask you a few clarifications before moving forward with this one. So far all I did was to reduce the amount for for-loops but the data changed in subtle ways that I'm not sure they are related to the code change. I believe it may due too the time of the query. |
6721f42
to
8c15d10
Compare
8c15d10
to
807f140
Compare
Where does this stand? Is there more work to be done before a review? |
Let's close and reopen to toggle the CIs. If it still pass, I'd like to check if the changed data is expected, due to a later run, or if my changes are responsible for it. Hard for me to say b/c I'm not familiar with MBON. |
closed and reopened to trigger checks |
@ocefpaf this now passes all checks. Are we good to merge? |
IMO, yes. It would be nice if you, or someone more familiar with mbon, could run it and comment if the results make sense. At the time I recall we had almost 100% comparison, with the exception of https://github.com/ioos/ioos_metrics/pull/81/files#diff-c1e3298f59885519b1b10f023299eae49fedc0616aaf818f2155ed5a11a5371bR607 |
Something isn't right with the dates, the data only goes back to 2023-04. I'm testing using the code in https://github.com/ioos/ioos_metrics/blob/main/notebooks/mbon_citation_visualizations.ipynb |
Thanks! Now I know were to start looking for code differences and try to fix this. |
Here's what the numbers should be:
OBIS downloads:
GBIF Downloads:
|
Start addressing #79