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

EXP: For big cube, be a minimalist #8

Closed

Conversation

pllim
Copy link

@pllim pllim commented May 1, 2024

Companion to:

Seems a little more performant when using spacetelescope#2827 instead of main with these changes, but slider still too laggy to be usable (but not as laggy as main). Subset creation at least possible (not possible on main).

🐱

kecnry and others added 2 commits April 30, 2024 16:02
and avoid loading uncert and correct units

[ci skip] [rtd skip]
@kecnry kecnry force-pushed the cubeviz-spec-extract-through-plugin branch 3 times, most recently from b7873f5 to a39bb5a Compare May 2, 2024 12:37
Copy link
Owner

@kecnry kecnry left a comment

Choose a reason for hiding this comment

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

If I understand the diff correctly, this skip parsing the hdulist if the cube is over 8 million spaxels? What are the consequences of that?

Comment on lines +256 to +260
if not is_big_cube:
sc = _return_spectrum_with_correct_units(
flux, wcs, metadata, data_type, hdulist=hdulist)
else:
sc = Spectrum1D(flux=flux, wcs=wcs, meta=metadata)
Copy link
Owner

Choose a reason for hiding this comment

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

will this also skip any necessary units stuff? Can we just set/pass hdulist=None and still use _return_spectrum_with_correct_units or is that still too expensive?

Copy link
Author

Choose a reason for hiding this comment

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

_return_spectrum_with_correct_units creates a new Spectrum1D object. I have not looked into whether it does a copy or what. I was just trying some possible low hanging fruits to see if anything helps. Given the ticket is only 2 points, I didn't dig too deep here. There is a chance we do not have to disable this (and I don't think we want to disable if we can avoid it).

@pllim
Copy link
Author

pllim commented May 2, 2024

this skip parsing the hdulist if the cube is over 8 million spaxels?

It skips parsing uncert and mask but flux should still be loaded.

@pllim
Copy link
Author

pllim commented May 2, 2024

My diff here is only exploratory. It didn't completely fix the lag in loading and slice. I tried profiling the code on main but Snakeviz crashed. I ran out of time so I didn't try Snakeviz here. See the ticket comments for a full report. Thanks!

@kecnry
Copy link
Owner

kecnry commented May 2, 2024

My diff here is only exploratory.

Gotcha 👍

@pllim
Copy link
Author

pllim commented May 2, 2024

Follow up tickets created. This is no longer necessary.

@pllim pllim closed this May 2, 2024
@pllim pllim deleted the pr-to-pr2827 branch May 2, 2024 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants