`FitsHdu::read_key` returns different values for `f32` and `i32` types

[art-den]

hdu.read_key::<f32>(fptr, "GAIN") returns Some(200.0) hdu.read_key::<i32>(fptr, "GAIN") returns Some(1)

FITS file is here: https://drive.google.com/file/d/1CalgywTpuKKXDqO8rwc31APoVSxv_c-R/view?usp=sharing

[simonrw]

You're absolutely right - confirmed with this cfitsio program:

#include <assert.h>
#include <fitsio.h>

int main() {
  fitsfile *fptr = NULL;
  int status = 0;

  const char *filename =
      "../examples/M_27_Light_30_secs_2022-06-29T00-53-28_024.fits";
  if (fits_open_file(&fptr, filename, READONLY, &status)) {
    fprintf(stderr, "error opening file\n");
    fits_report_error(stderr, status);
    return status;
  }


  float floatval = 0;
  if (fits_read_key(fptr, TFLOAT, "GAIN", &floatval, NULL, &status)) {
      fprintf(stderr, "reading float key\n");
      fits_report_error(stderr, status);
      return status;
  }

  int intval = 0;
  if (fits_read_key(fptr, TINT, "GAIN", &intval, NULL, &status)) {
      fprintf(stderr, "reading int key\n");
      fits_report_error(stderr, status);
      return status;
  }

  printf("Got float value %f, int value %d\n", floatval, intval);


  if (fits_close_file(fptr, &status)) {
    fprintf(stderr, "error closing file\n");
    fits_report_error(stderr, status);
    return status;
  }

  assert(status == 0);

  return 0;
}

Thanks for raising, I'll take a look 👍

[simonrw]

It looks like it's only reading i32 values from the header that is broken.

Looking through the docs, I see the name of the cfitsio function that reads an integer is fits_read_key_log which screams "logical" to me i.e. boolean. That would explain why you get the value of 1. In my testing, I found the header value returned is 1 also, so that might explain it.

I think instead the better option is to remove the ability to read i32 values, so the caller must read i64 values.

I'm interested to gauge whether this is sensible or not. While I'm at it I might remove the ability to read f32 values as well. I see two approaches:

1. support i32 reading correctly, by most likely reading the value as an i64 and casting, which is potentially truncating if done naively, or may return an Err if the value does not fit into an i32, or
2. remove support for i32 (and as a byproduct f32) since it's unlikely the caller needs an i32 vs an i64.

Image or table data I can imagine caring deeply about the bit-size of the result since I may be reading thousands of pixels and so decreasing the number of bytes by two is preferable. For header cards however, it's unlikely they will be read in in such large quantities to justify using a smaller bit-size.

@art-den @cjordan what are your thoughts?

[cjordan]

Yep, this is a pain. As I discovered a few years ago now, rust-fitsio's behaviour is consistent with cfitisio, and my workaround was to read everything as a string, then let Rust parse it:

https://github.com/MWATelescope/mwalib/blob/a807114b8c1051c5bf7c1af59a38e960c2eeca16/src/fits_read/mod.rs#L338-L390

Should we change things in rust-fitsio? I think if we continue to let callers read values as primitives, we should not; after all, how are we supposed to protect users from reading floats as ints, etc.? On the other hand, we could adopt the approach of reading everything as a string then parsing it, but that would be a significant overhaul.

[art-den]

Looking through the docs, I see the name of the cfitsio function that reads an integer is fits_read_key_log which screams "logical" to me i.e. boolean.

Hm... May be better to use fits_read_key_lng for i32 values? It returns value of long c type

[art-den]

You're absolutely right - confirmed with this cfitsio program:

What is you result? Mine is Ok

Got float value 200.000000, int value 200

[simonrw]

You're absolutely right - confirmed with this cfitsio program:

What is you result? Mine is Ok

Got float value 200.000000, int value 200

Yeah I get the same. When I use fits_read_key_log it returns 1 confirming my suspicions.

To @cjordan's point - both cfitsio and rust-fitsio assume the user knows the type of the header value they are trying to access. This is not unreasonable I suppose, but perhaps a bit frustrating.

I had toyed with the idea of understanding what the type of the header was and returning something type-aware, e.g. an enum

enum HeaderValue {
    Int(i64),
    Float(f64),
    String(String),
    Bool(bool),
}

then the user gets the type of the key, however they have to match each time which also may be a pain.

@cjordan your function is conceptually similar - the user has to determine the type of the header value, but using std::str::FromStr rather than fitsio::ReadsKey.

I think the best bet is to remove the read_key::<i32> implementation (via deprecating it first) and forcing the user to read an i64 (and similarly an f64 for floats). I think this exercise has shown me that I didn't understand what fits_read_key_log did.

[simonrw]

Annoyingly I can't mark the read_key::<i32> and read_key::<f32> methods as deprecated, because annotating trait impls is not supported :(

[simonrw]

@cjordan @art-den: are either of you reading header values as i32 or f32 much? I'm going to remove the ability to read as these types going forward, but want to make sure I'm not going to break your projects at least!

[art-den]

It's Ok for me. I will simply rewrite little part of my code. But there are more repositories with fitsio in Cargo.toml on github: https://github.com/search?q=filename%3ACargo.toml+fitsio

[cjordan]

@mindriot101 I appreciate the concern! There might be some things that break, but as long as semver is honoured, I'm happy for the changes to proceed. I don't think I ever read i32 but honestly, I'm weary about cfitsio's reading of f32 anyway so reading f64 then demoting seems safer anyway.

[simonrw]

@cjordan what are you weary about with cfitsio's reading of f42s?

I might just fix i32 reading, since it may be more convenient for the user if they do 32-bit arithmetic on the value to not cast it. This will leave i32 and f32 reading available

It might be good to perform a read that knows the type so the user gets the type back, I'll do some research

[cjordan]

@mindriot101 I'm probably overthinking it, but due to these surprises when reading fits keys, I've kept a (un)healthy amount of scepticism when reading in floats. In my work, we care a lot about precision, so if there's a chance that reading a f64 and demoting it is somehow better than reading an f32 directly, I'll probably do that. Fortunately in most cases f32 values are reserved for the bulk of our data, not metadata where I'd be reading fits keys.

[simonrw]

Looks like I removed the f32 and i32 header card reading in #170 so I'll close this issue. Thanks for your input!

[simonrw]

...erm i misread the output of the github cli - ignore me :facepalm: