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

Patch refactor #180

Merged
merged 82 commits into from
Jul 6, 2023
Merged

Patch refactor #180

merged 82 commits into from
Jul 6, 2023

Conversation

d-chambers
Copy link
Contributor

@d-chambers d-chambers commented Jun 30, 2023

Description

This is a HUGE pull request. It refactors many (most) of DASCore's internals. Despite the immensity of the changes, I don't think it introduces any significant backward incompatibilities. Here are a few highlights:

  1. Implements numpy-based coordinates (dascore.core.coords/coordmanager) rather than using xarray.DataArray. This gives us more control over the patch/data/coordinates and simplifies and speeds up lots of other code. Xarray is now an optional dependency.
  2. Implements first-class support for units using the pint library.
  3. Refactors merging/chunking for fairly large performance improvements.
  4. Refactors most of the parsers to use DASCore coordinates and ensure their read/scan operations return the same min/max values.
  5. Better string representation of objects

There are also significant speed ups as shown in the following snapshot from the asv benchmarks. Patch, merge, chunk, and select are now significantly faster. Generating string representations for Patches and Spools, however, is not about 60% slower. This is largely due to the addition of rich styling, and probably wont be a burden for most codes since calling __str__ on a large number of DASCore objects is not likely a common use.

image

Here are a few things left to do:

  • Full doc page on coordmanager (probably in dascore.core.coordmanager)
  • More tests with units and processing functions; particularly pass_filter and taper.
  • Merge tests for patches with various types of coordinates (monotonic, evenly sampled, no restraints)
  • Update docs for Patch to explain string repr. and coordinates
  • Check coverage, clean up dead code.
  • Add a minimal deps test to CI, probably just for linux.
  • Create a release before merging, so there is a nice checkpoint to refer users to if anything breaks (while we fix it)

Checklist

I have (if applicable):

  • referenced the GitHub issue this PR closes.
  • documented the new feature with docstrings or appropriate doc page.
  • included a test. See testing guidelines.
  • your name has been added to the contributors page (docs/contributors.md).
  • added the "ready_for_review" tag once the PR is ready to be reviewed.

@codecov
Copy link

codecov bot commented Jun 30, 2023

Codecov Report

Merging #180 (611519e) into master (26d7961) will increase coverage by 1.00%.
The diff coverage is 99.44%.

@@            Coverage Diff             @@
##           master     #180      +/-   ##
==========================================
+ Coverage   96.77%   97.78%   +1.00%     
==========================================
  Files          58       64       +6     
  Lines        3627     4916    +1289     
==========================================
+ Hits         3510     4807    +1297     
+ Misses        117      109       -8     
Flag Coverage Δ
unittests 97.78% <99.44%> (+1.00%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
dascore/viz/__init__.py 100.00% <ø> (ø)
dascore/viz/wiggle.py 79.59% <ø> (ø)
dascore/io/quantx/core.py 96.55% <50.00%> (-3.45%) ⬇️
dascore/utils/patch.py 97.04% <95.12%> (-0.79%) ⬇️
dascore/core/spool.py 98.96% <95.74%> (+0.21%) ⬆️
dascore/proc/units.py 97.61% <97.61%> (ø)
dascore/utils/models.py 98.13% <98.13%> (ø)
dascore/utils/display.py 98.91% <98.91%> (ø)
dascore/__init__.py 100.00% <100.00%> (ø)
dascore/clients/dirspool.py 100.00% <100.00%> (ø)
... and 32 more

... and 2 files with indirect coverage changes

@d-chambers d-chambers added the no_ci disables CI label Jul 2, 2023
@d-chambers d-chambers removed the no_ci disables CI label Jul 4, 2023
@d-chambers
Copy link
Contributor Author

All green and checklist complete.

@d-chambers d-chambers merged commit 2014460 into master Jul 6, 2023
@d-chambers d-chambers deleted the patch_refactor branch July 6, 2023 23:47
@d-chambers d-chambers mentioned this pull request Jul 6, 2023
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.

1 participant