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

Massive PR, datamodel change. #68

Merged
merged 53 commits into from
Mar 3, 2021
Merged

Massive PR, datamodel change. #68

merged 53 commits into from
Mar 3, 2021

Conversation

brisvag
Copy link
Owner

@brisvag brisvag commented Feb 26, 2021

This PR started as a rework of some classification function, and soon it was clear that many things in the datamodel and project structure needed fixing. I changed a lot of stuff, but here's a (probably incomplete) writeup:

  • changed to a data strcture with DataSet at its core. DataBlocks are now directly contained in it, and have themselves a reference to what "volume" (previously DataCrate object) they belong to. This greatly simplify looping and such, since we're not working with an actual nested structure.
  • naming of blocks and volumes is now used as unique identifiers, which will make it easy for merging volumes and avoid datablock conflicts
  • previous DispatchList functionality is still present, but only when needed (not directly on the dataset itself, but for example when calling DataSet.particles)
  • depictors model changed with the rest: now only datablocks have depictors; they can have multiple depictors, and multiple modes (for example, for plots)
  • minimal mesh depiction
  • classification now does ome vectorised fancy stuff. Some of the representation is slighly broken as is due to the big changes to depictor structure, but it works
  • removed a lot of legacy code that was useless (all the merge/stack stuff, for example)

Tests work but things may have broken where tests don't cover... will need some more time to figure it out!

@codecov
Copy link

codecov bot commented Feb 26, 2021

Codecov Report

Merging #68 (271f2c3) into develop (bb34142) will increase coverage by 1.54%.
The diff coverage is 68.83%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #68      +/-   ##
===========================================
+ Coverage    68.51%   70.06%   +1.54%     
===========================================
  Files           85      101      +16     
  Lines         1639     1974     +335     
===========================================
+ Hits          1123     1383     +260     
- Misses         516      591      +75     
Impacted Files Coverage Δ
peepingtom/__main__.py 0.00% <ø> (ø)
peepingtom/gui/__init__.py 0.00% <ø> (ø)
peepingtom/gui/gui.py 0.00% <ø> (ø)
peepingtom/gui/particle_selector.py 0.00% <0.00%> (ø)
peepingtom/io_/utils/generic.py 100.00% <ø> (+35.00%) ⬆️
peepingtom/io_/utils/star_conventions.py 100.00% <ø> (ø)
peepingtom/io_/write/star/star.py 95.83% <ø> (ø)
peepingtom/io_/read/read.py 18.29% <27.77%> (-1.19%) ⬇️
peepingtom/depictors/viewer.py 28.12% <28.12%> (ø)
peepingtom/depictors/pyqtgraph/plotdepictor.py 35.00% <35.00%> (ø)
... and 119 more

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 f54cd03...271f2c3. Read the comment docs.

Copy link
Collaborator

@alisterburt alisterburt left a comment

Choose a reason for hiding this comment

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

Looks good overall - seems like working with datasets will be easier with this architecture! Few tiny questions but nothing blocking, happy for you to merge if you're happy!

peepingtom/depictors/napari/naparidepictor.py Outdated Show resolved Hide resolved
peepingtom/io_/read/read.py Show resolved Hide resolved
peepingtom/io_/read/star/reader_functions.py Outdated Show resolved Hide resolved
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