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

237 increase page size #239

Merged
merged 6 commits into from
Nov 9, 2021
Merged

237 increase page size #239

merged 6 commits into from
Nov 9, 2021

Conversation

trey-stafford
Copy link
Contributor

Looks like there are still some references to a page size of 10 in jupyter notebooks. I have not updated those, but can do so if useful!

@github-actions
Copy link

github-actions bot commented Nov 2, 2021

Binder 👈 Launch a binder notebook on this branch for commit 6500cad

I will automatically update this comment whenever this PR is modified

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

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

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

@trey-stafford
Copy link
Contributor Author

Associated issue: #237

@trey-stafford
Copy link
Contributor Author

Note that the failing test appears to be an issue on the development branch as well.

@JessicaS11
Copy link
Member

@trey-stafford Thanks for this update from NSIDC! If you're able to update the notebooks to match the updated code, that would be awesome and much appreciated. I added in @weiji14's test fix (#241), so hopefully updating the base branch will fix the error.

@codecov-commenter
Copy link

Codecov Report

Merging #239 (a0f63a3) into development (7f38707) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##           development     #239   +/-   ##
============================================
  Coverage        54.94%   54.94%           
============================================
  Files               20       20           
  Lines             1547     1547           
  Branches           321      321           
============================================
  Hits               850      850           
  Misses             639      639           
  Partials            58       58           
Impacted Files Coverage Δ
icepyx/core/APIformatting.py 71.97% <ø> (ø)
icepyx/core/query.py 39.06% <ø> (ø)
icepyx/tests/test_visualization.py 100.00% <ø> (ø)
icepyx/tests/test_APIformatting.py 97.05% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7388316...a0f63a3. Read the comment docs.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@trey-stafford
Copy link
Contributor Author

@trey-stafford Thanks for this update from NSIDC! If you're able to update the notebooks to match the updated code, that would be awesome and much appreciated. I added in @weiji14's test fix (#241), so hopefully updating the base branch will fix the error.

@JessicaS11 this commit corrects what I believe is the only remaining reference in the 'examples' notebooks: 9cf10fa. I've left the 'dev' notebooks untouched for now (doc/source/dev-notebooks/).

There's still a reference to using a smaller page size in examples/examples/ICESat-2_DAAC_DataAccess_Example.ipynb because it refers to synchronous requests. Note that this notebook indicates that synchronous requests are done by default, but I think this must be outdated. It looks like the request mode is async by default (https://github.com/icesat2py/icepyx/blob/development/icepyx/core/APIformatting.py#L446).

Given that the page size limit is only 100 when the request mode is synchronous, maybe we need another check somewhere that decreases the page size when a user overrides that default and uses synchronous mode?

@JessicaS11 JessicaS11 linked an issue Nov 9, 2021 that may be closed by this pull request
@JessicaS11
Copy link
Member

@trey-stafford I merged #240, so now this is out of data with the base. I also sent you an invite to the icesat2py organization so you can create a branch to submit PRs in the future and any reviewer can update from the base.

@JessicaS11 JessicaS11 merged commit 5b8112d into icesat2py:development Nov 9, 2021
JessicaS11 added a commit that referenced this pull request Dec 8, 2021
* add github action to add binder badge to PRs (#229)
* use the binder badge action directly (instead of a manual implementation of it) (#233)
See the discussion in #230 for more details on this switch.
* preliminary AWS access (#213)
* update links for travis badge (#234)
* Fix failing test_visualization_date_range check for ATL07 (#241)
* By default, no email status updates to users when ordering granules (#240)
* remove extra cell causing errors in example notebook
* Set default page size for orders to 2000 per NSIDC recommendation (#239)
* Add ICESat-2 data read-in functionality (#222)
* update examples from 2020 Hackweek tutorials
* update add and commit GitHub Action version (and UML diagrams) (#244)
* merge traffic (GitHub and PyPI) and bib updates (#245)
* Release 0.5.0 (#246)
* release v0.5.0 CI fixes (#251)
* fix Travis CI label in readme
* update earthdata login fixture for testing
* add required input to pytest fixture (#252)

Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
Co-authored-by: trey-stafford <trey.stafford@colorado.edu>
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.

Default order page size is too small
3 participants