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.

Sign up for GitHub

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

Jump to bottom

Support creating fitsfiles from raw pointers #195

Merged
merged 9 commits into from
Dec 19, 2022
Merged

Support creating fitsfiles from raw pointers #195

merged 9 commits into from
Dec 19, 2022

Conversation

simonrw
Copy link
Owner

@simonrw simonrw commented Dec 3, 2022 •
edited
Loading

Closes #194

@simonrw simonrw self-assigned this Dec 3, 2022
@simonrw simonrw force-pushed the fix/194/upgrade-from-fitsfile-pointer branch 4 times, most recently from 65700f7 to 81a5cd2 Compare December 5, 2022 23:05
@simonrw simonrw mentioned this pull request Dec 5, 2022
Closed
@simonrw simonrw marked this pull request as ready for review December 6, 2022 20:47
@simonrw simonrw force-pushed the fix/194/upgrade-from-fitsfile-pointer branch from 81a5cd2 to bacbd62 Compare December 6, 2022 20:48
@simonrw simonrw requested a review from cjordan December 18, 2022 20:06
@cjordan
Copy link
Collaborator

cjordan commented Dec 18, 2022

I haven't run the code myself, but I trust that it works, and it looks fine to me. My only real comment is to do with the mandatory filename field of the FitsFile struct; it's a nitpick, but strictly speaking it shouldn't be necessary for the new method. The situation could be "fixed" by making the filename field optional, and the new method has an filename: Option<impl AsRef<Path>> field. However, this doesn't sit with me 100% well (I guess I find optional arguments a little untidy), so you could just ignore this nitpick and proceed 🙂

@simonrw simonrw force-pushed the fix/194/upgrade-from-fitsfile-pointer branch from 46a03a1 to dabece5 Compare December 19, 2022 11:28
@simonrw
Copy link
Owner Author

simonrw commented Dec 19, 2022

I haven't run the code myself, but I trust that it works, and it looks fine to me. My only real comment is to do with the mandatory filename field of the FitsFile struct; it's a nitpick, but strictly speaking it shouldn't be necessary for the new method. The situation could be "fixed" by making the filename field optional, and the new method has an filename: Option<impl AsRef<Path>> field. However, this doesn't sit with me 100% well (I guess I find optional arguments a little untidy), so you could just ignore this nitpick and proceed 🙂

Thanks for pointing this out. I hadn't really considered that, nor that the filename is actually optional for the FitsFile struct.

I think we can get away without needing the optional filename for opening/creating a disk-backed file. dabece5 shows how we can require the field for create/open/edit, and not require it for from_raw. It's optional for FitsFile but it's not currently used. We only really use it for printing which I expect to be a niche feature. We may even want to make the property non-pub to emphasise that the user shouldn't really care, or more accurately: they should keep track of the filename themselves.

Update: I've done this in a new commit 3342d16...

@simonrw
Copy link
Owner Author

simonrw commented Dec 19, 2022

I think I'll merge this, but I'll write up a changelog entry first. Hopefully nobody is depending on the FitsFile.filename field...

@simonrw
Support creating fitsfiles from raw pointers
c007981 
This is an unsafe operation and has been marked as such.
@simonrw
wip: example of reading from memory
f00aaa6
@simonrw
add example of reading fits file from memory
6a937e8
@simonrw
don't run example on non-linux 64-bit systems
4ebfb8e
@simonrw
Re-export fitsio-sys as part of fitsio
6195a30 
I was getting conflicts between the versions of `fitsio-sys` that were
included in the crate compilation. Rust only allows one crate that links
with a system library to be included in the list of crates.

By exporting `fitsio-sys`, we can ensure that if the user needs
`fitsio-sys`, then it's the same version that's used by `fitsio` and
there will be no conflict.
@simonrw
Assume example is running from the root directory
d2cfd04
@simonrw
Mark uses as only for demo platform
f6b4be4 
This removes some warnings
@simonrw
Don't require a filename
4d840a1 
Not every fits file has a filename. We don't use the filename for
anything in particular, so make it optional.
@simonrw
Remove filename as public property
dcf2436 
People should not be using this field themselves. It's only for us if
pretty-printing is used.
@simonrw simonrw force-pushed the fix/194/upgrade-from-fitsfile-pointer branch 2 times, most recently from 37e251b to dcf2436 Compare December 19, 2022 22:41
@simonrw simonrw merged commit 86b2a02 into main Dec 19, 2022
@simonrw simonrw deleted the fix/194/upgrade-from-fitsfile-pointer branch December 19, 2022 22:47
@cjordan
Copy link
Collaborator

cjordan commented Dec 19, 2022

Your changes match what I would've done! Looks good. I didn't spot that filename was a public field, but I'd argue that it should not be; being public means someone could change it, and while in this case it wouldn't do anything bad, it's better IMO to not make this possible to prevent someone even considering it.

Just a friendly reminder that we're using rust-fitsio across all our archive at the MWA collaboration, processing tens of petabytes!

@simonrw
Copy link
Owner Author

simonrw commented Dec 20, 2022

That's awesome, glad to hear the crate is being battle tested!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjordan cjordan Awaiting requested review from cjordan

Assignees

@simonrw simonrw

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Opening FITS-files from memory
2 participants
@simonrw @cjordan