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

Update viz doc to clarify its current status (partial functionality - incompatible with OA API changes) #540

Merged
merged 5 commits into from
Aug 14, 2024

Conversation

mfisher87
Copy link
Member

@mfisher87 mfisher87 commented Aug 8, 2024

Related: #454

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link

github-actions bot commented Aug 8, 2024

Binder 👈 Launch a binder notebook on this branch for commit b1e9b5f

I will automatically update this comment whenever this PR is modified

Binder 👈 Launch a binder notebook on this branch for commit 632ee42

Binder 👈 Launch a binder notebook on this branch for commit 9c9e722

Binder 👈 Launch a binder notebook on this branch for commit d664b80

Binder 👈 Launch a binder notebook on this branch for commit cb195f9

@mfisher87 mfisher87 closed this Aug 8, 2024
@mfisher87 mfisher87 reopened this Aug 8, 2024
Copy link

codecov bot commented Aug 8, 2024

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 65.98%. Comparing base (f2ad0a8) to head (cb195f9).
Report is 42 commits behind head on development.

Files with missing lines Patch % Lines
icepyx/core/visualization.py 0.00% 4 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           development     #540      +/-   ##
===============================================
- Coverage        66.05%   65.98%   -0.07%     
===============================================
  Files               36       36              
  Lines             3049     3052       +3     
  Branches           537      538       +1     
===============================================
  Hits              2014     2014              
- Misses             947      950       +3     
  Partials            88       88              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mfisher87
Copy link
Member Author

Thoughts on the coverage change? The function I changed is currently not covered, but I can make it covered. I'd probably mock the response and verify a 400 raises and a 200 doesn't, but my personal feeling is there's not a ton of value there :)

@JessicaS11
Copy link
Member

Thoughts on the coverage change? The function I changed is currently not covered, but I can make it covered. I'd probably mock the response and verify a 400 raises and a 200 doesn't, but my personal feeling is there's not a ton of value there :)

Agreed. Since #454, the broader conversation around this module is that given the lack of response to #495 and minimal complaints that this module stopped working, it may make sense to rethink how we offer any quick data visualization within icepyx.

@mfisher87 mfisher87 changed the title Update viz doc to clarify its current status (nonfunctional while OA API is unavailable) Update viz doc to clarify its current status (incomplete while OA API is unavailable) Aug 8, 2024
@mfisher87
Copy link
Member Author

mfisher87 commented Aug 12, 2024

From #454, perhaps we can fix the notebook by updating the URL. I have no idea if the API has breaking changes or not yet 😬

EDIT: It does! Moving on... :)

@mfisher87 mfisher87 changed the title Update viz doc to clarify its current status (incomplete while OA API is unavailable) Update viz doc to clarify its current status (partial functionality - incompatible with OA API changes) Aug 12, 2024
Copy link
Member

@JessicaS11 JessicaS11 left a comment

Choose a reason for hiding this comment

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

I like the updates to the warning too - thanks @mfisher87!

@mfisher87 mfisher87 merged commit 5825e51 into development Aug 14, 2024
5 of 7 checks passed
@mfisher87 mfisher87 deleted the callout-on-nonfunctional-doc branch August 14, 2024 18:51
@mfisher87
Copy link
Member Author

Thanks for the review! 🪨 ⭐

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.

2 participants