fits_get_colnum should be case-sensitive

[hbouy]

It seems that fits_get_colnum is set-up to be always case insensitive. I know a fair number of astronomical catalogs that use case-sensitive column names. The Gaia DR2 catalog (as offered on CDS ViZieR) released yesterday is one of them (e.g. the b_Teff and B_Teff columns). It would be great to make it case sensitive by default. thanks

[mweastwood]

Does changing 0 to 1 on this line here get you the behavior you're looking for?

https://github.com/JuliaAstro/FITSIO.jl/blob/2ded0179af50a731c97a9bae384535659ff53838/src/libcfitsio.jl#L940

Case sensitivity seems like a sensible default, but at the very least this should be configurable.

[giordano]

Case sensitivity seems like a sensible default, but at the very least this should be configurable.

What about a keyword argument of Bool type?

[kbarbary]

Agreed. It should be easy to add a Bool parameter or keyword arg to fits_get_colnum. I don't know why the default 0 (case insensitive) was chosen. I agree that case-sensitive would be a better default. Changing the default is technically breaking, but I'm not sure there are so many users that it is worth figuring out how to issue a good deprecation warning. What do you all think?

In the high level interface, we should add a keyword argument case_sensitive=true to read(::TableHDU, ::String). Alternatively we could have a global (module-wide) configuration setting. This might make sense as users will probably wish to read many columns with the same setting.

[mweastwood]

I think if the user wants to default to a different value of case_sensitive, there's no reason they can't define: myread(hdu, name) = read(hdu, name, case_sensitive=false). I think we are likely to regret a global setting.

[giordano]

I think we are likely to regret a global setting.

I'm not sure what you mean here, could you please elaborate?

[mweastwood]

I don't know if libcfitsio itself is thread-safe, but if we have any ambition of FITSIO.jl being thread-safe, we should prefer to avoid these global configuration settings. The basic idea is one thread might set case_sensitive=true and another thread sets it back to false before the first thread was able to call read. So now you need a lock to prevent this setting from being changed by another thread before you're ready, but now only one thread can call read at a time.

Furthermore even if case_sensitive is immutable and never changed at runtime, I dislike global settings like this because it makes code less portable. If I copy my code to your machine and we've selected different values of case_sensitive, my code will have different behavior and it won't be obvious why.

[giordano]

A global setting can be made thread-safe, I've already done something similar recently. I'm not sure if I 100% agree with it, but I see your point about portability. In #99 I'm fixing the reported issue with a simple optional keyword only.

[kbarbary]

Good points @mweastwood.

[giordano]

Apparently, this is a breaking change for Celeste :sweat_smile: See https://github.com/JuliaLang/METADATA.jl/pull/18001

[kbarbary]

While we are allowed to make breaking changes like this, I'm wondering if this change might be disruptive enough to warrant a deprecation warning. There might be enough FITSIO users at this point to make it worthwhile.

However, the deprecation warning would have to be done carefully to not be annoying. E.g., only warn when case-insensitive returns a match but case-sensitive does not. I think it's fine to only do this in the high-end level interface.

On Fri, Sep 14, 2018, 12:22 PM Mosè Giordano notifications@github.com wrote:

Apparently, this is a breaking change for Celeste 😅 See JuliaLang/METADATA.jl#18001 https://github.com/JuliaLang/METADATA.jl/pull/18001

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/JuliaAstro/FITSIO.jl/issues/98#issuecomment-421459501, or mute the thread https://github.com/notifications/unsubscribe-auth/AA7Nl_UUA9CG-OI33YD1F494_t7GQWc-ks5ubAHugaJpZM4TkdA1 .

[giordano]

Anyway I'm working on a fix in Celeste to use the names with proper capitalization.

Regarding the depwarn, would that require a try-catch?