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

Improvement suggestions #18

Open
electro-logic opened this issue May 17, 2024 · 9 comments
Open

Improvement suggestions #18

electro-logic opened this issue May 17, 2024 · 9 comments

Comments

@electro-logic
Copy link

electro-logic commented May 17, 2024

Hello

Here some suggestions:

  • HeaderEntry ToString() override to make debugging easier
  • PrimaryHdu.Header.Entries.AddRange()
  • Sync version of Async methods (ex. Write and WriteAsync)
  • Shortcut method to directly write an array (in doc.PrimaryHdu.Data)
  • PrimaryHdu is not available in the non-generic FitsDocument class
  • The default HeaderEntry are missing comments (ex. SIMPLE, NAXIS, etc..)

And if you would like to cite in the Readme file, AstroCapture (Xamarin Android app) is using this library

@RononDex
Copy link
Owner

Thanks for the feedback!

HeaderEntry ToString() override to make debugging easier
PrimaryHdu.Header.Entries.AddRange()
Sync version of Async methods (ex. Write and WriteAsync)

Those are certainly a good idea, will add that in the next release!

Shortcut method to directly write an array (in doc.PrimaryHdu.Data)

That is something I have already in mind, not sure how I will implement this yet though

PrimaryHdu is not available in the non-generic FitsDocument class

There is a reason for that: The PriamryHdu is of type ImageHeaderDataUnit<T>. The generic is important here.
If you initialize the document without generics, it can not know what T is and therefor can not give you the strong typed PrimaryHdu property. You instead have to access it through fitsFile.HeaderDataUnits[0]

I could add a non-generic ImageHeaderDataUnit maybe, however I am not sure how this could be implemented yet.

The default HeaderEntry are missing comments (ex. SIMPLE, NAXIS, etc..)

That is also on purpose, Comments are optional. However, to make it more verbose I guess I could add some comments to them (I don't see the use of it though)

@RononDex
Copy link
Owner

Sorry forgot this part:

And if you would like to cite in the Readme file, AstroCapture (Xamarin Android app) is using this library

Yes! I will gladly do so :)

@electro-logic
Copy link
Author

Hello,

Thank you for your replies,

Shortcut method to directly write an array (in doc.PrimaryHdu.Data)
That is something I have already in mind, not sure how I will implement this yet though

I'm now using myarray.AsMemory().Span.CopyTo(doc.PrimaryHdu.Data.RawData.Span), maybe something like this can be exposed as a method for cleaner calling code

The default HeaderEntry are missing comments (ex. SIMPLE, NAXIS, etc..)
That is also on purpose, Comments are optional. However, to make it more verbose I guess I could add some comments to them (I don't see the use of it though)

I have added header entries with comments, but the builtin entries are missing comments and someway this make the file "inconsistent" from an user point of view

Thanks again

@RononDex
Copy link
Owner

RononDex commented May 19, 2024

You could add comments to the default header entries by doing Header["Simple"].Comment = "SomeComment"

@electro-logic
Copy link
Author

electro-logic commented May 20, 2024

Hello,

Thanks for your reply,

In the nuget package the Comment property is not accessible with the code
doc.PrimaryHdu.Header["Simple"]
or
doc.HeaderDataUnits[0].Header
because the returned type is object

As a workaround I now have the LINQ code
doc.PrimaryHdu.Header.Entries.Where(e => e.Key == "Simple").First().Comment

Why not using a Dictionary instead of a List for Header entries?

@RononDex
Copy link
Owner

Ah yes that is true, you can not change the .Comment through .Header["xxx"], will have to see if I can change that.

And Dictionaries require unique keys, which does not work for HEADER entries. Several keys can have multiple entires (like COMMENT, HISTORY, etc)

@electro-logic
Copy link
Author

If there are multiple entries with the same key, a good solution can be to use
Dictionary<string, List>
so the relationship one-to-many is explicit and the lookup faster

@RononDex
Copy link
Owner

Another issue with Dictionaries are, that they do not retain order.
In fits files the order of some entries is important, and the rest is up to the user (users might store their header entries in a certain order on purpose). The fits file standard says that ordering should be retained when reading and writing the file.

@electro-logic
Copy link
Author

Ok, better to keep entries as a list then. Thank you for the explanation.

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

No branches or pull requests

2 participants