Update to SOFA 2015-02-09 by timj · Pull Request #26 · liberfa/erfa

timj · 2015-02-25T18:10:24Z

Syncs up ERFA with release 2015-02-09 of SOFA.

I have attempted to retain the local patch to dat.c.

The change looks larger than it really is because of the change in copyright date.

SOFA release notes:

[New Material] 2 routines, iauG2icrs and iauIcrs2g, have been released for the new Galactic Coordinates section. They allow transformations to be made between the 1958 IAU galactic coordinate system and the ICRS.
[Update/Change] Updated iauDat to account for the leap second at the end of June 2015.
[Update/Change] Changes to iauC2ixys, iauGc2gde, iauRm2v and iauRv2m. There was a test for zero that could potentially cause compiler warnings. The test has been changed.
[Update/Change] Changes to sofa.h. Addition of the new functions, removal of duplicate functions. All functions are listed in the categories that are given on the website and in the manual.
[Update/Change] Update to t_sofa_c.c to accommodate the new routines.

astrofrog · 2015-02-25T18:22:15Z

@eteq - I guess it would make sense to start using the Galactic coordinates defined here since they are IAU sanctioned (once we merge into Astropy).

astrofrog · 2015-02-25T18:22:21Z

@timj - thanks for the PR!

astrofrog · 2015-02-25T18:28:25Z

The new galactic stuff is really good - it means we finally have an IAU-sanctioned path from ICRS to Galactic that doesn't depend on FK4. The start of a new era of precision Galactic coordinates? :)

eteq · 2015-02-25T18:51:09Z

@astrofrog - I'm not 100% sure we want to switch to using this... the problem isn't so much the lack of an IAU standard now as much as the lack of a standard in the past. That is, if ERFA's approach to e.g. the e-terms than what has been assumed in most cases up until now, we may want to leave in our implementation. Will have to look more closely.

Regardless, thanks @timj for doing this - will review now.

astrofrog · 2015-02-25T19:05:16Z

@eteq - I think that for ICRS <-> Galactic there were several different conventions before anyway so changing to the IAU sanctioned one would be good (no one trusts old galactic coordinates to better than a fraction of an arcsec anyway). In fact, we should probably make sure everyone switches to that specific convention for the future.

For e-terms and things like that, we've actually been doing pretty well compared to other codes, so I'd be surprised if the IAU functions give different results from most other codes.

We should check all this though!

eteq · 2015-02-25T19:29:07Z

This change undoes #14, which is a fix we want. My impression was that this was supposed to be included in SOFA, but apparently not... but either way, we do not want to make this particular change. So can you just put back the
da = changes[(i < 0) ? 0 : i].delat; that was there before?

I looked at that but you had already put in the line 228 change that cause the function to return early if i is negative so why are we checking the variable again immediately afterwards. This way we minimize the changes between ERFA and SOFA. It would be easier if there was a branch for "pure" SOFA->ERFA and then a branch with the local changes. Then merges would be done by git rather than manually trying to keep them straight.

@timj - @mdboom should have a little bit more insight as the author of #14 . I think the main point was to avoid compiler warnings that were causing problems somewhere. (And this is the sort of thing we are willing to keep separate between ERFA and SOFA.)

Even if we decide to drop this change, though, the README would need to be updated to reflect this change.

@timj: We only need one of the fixes here. The return if i is negative is the better one (but should still probably be upstream in SOFA if it isn't -- don't have time to check right now).

Exactly. That's why I only kept the first check for a negative number.

The change was added in 506f302 which is part of the previous SOFA release. It looks to me like they edited the current dat.c from an old version of the source and not the "2013-12-02_d" release. The version of dat.c here does include the previous patch and the new version and correctly drops the local ERFA hack. I'll send a note to see if the dropping of the -5 error condition was deliberate. I will also update the README to say that the local patch is no longer needed (at least in the stated form).

Ok, sounds good. If there ends up being no local change necessary at all, then you can just remove the section about that change from the README entirely.

eteq · 2015-02-25T19:37:33Z

@astrofrog - definitely, and it's not really relevant until this is merged, anyway.

eteq · 2015-02-25T23:50:35Z

@timj - actually, I just realized something relevant to the discussion of what to do with configure.ac: that's really meant to be changed upon release. So it might just be easier to not include it in this PR at all (along with not changing the version number in the README).

(That said, it probably makes sense to do a release including this shortly after merging and I can always update it then, so it doesn't matter that much.)

timj · 2015-02-25T23:53:05Z

Ah. You are right. It's not really my job to bump the version numbers. That's the job of the release manager. I'll remove the configure.ac change.

The compiler warning in dat.c was fixed in commit 506f302

timj · 2015-02-26T17:34:40Z

I've pushed the change to the README and reverted the configure.ac changes. Have not heard anything from SOFA yet but I've retained the fix in dat.c as was in the previous release of SOFA.

eteq · 2015-02-26T18:42:59Z

Ok, sounds good @timj. Lets give, say a week or so to hear what SOFA thinks on this? (Perhaps pinging @scottransom will help grease the wheels a bit?)

scottransom · 2015-02-26T22:03:10Z

@timj: Who did you contact at SOFA? I haven't heard about this at all. Patrick Wallace is the person who is doing most of the actual code development. If you don't mind, can you please forward to me what you sent? I can definitely try to nudge as appropriate.

timj · 2015-02-26T22:14:28Z

I used the contact email on the SOFA web page. I'm more than happy to email Pat directly -- we know each other well.

The initial SOFA release had inadvertently removed a prior compiler warning fix.

timj · 2015-02-27T16:10:25Z

SOFA inform me that it was a mistake and a new release will be made shortly. Pat has sent me the corrected version which I've now included in this PR.

eteq · 2015-02-27T20:51:37Z

Ok, great. I think this is good except for one thing: should we wait until the official SOFA release with that fix so that we can make it clear this is based on that version? @timj, did you get a sense of whether "shortly" means a few days vs. a few months?

timj · 2015-02-27T21:03:50Z

The modified code has been submitted by Pat. The reply I got said "not too distant future". I don't see any harm in releasing this version as is -- it does have the fix from the previous SOFA release in it that is going to be in the next release so this version is technically less broken than the official version but in a way that has been approved by SOFA. I can add a little note to the README if you want to be explicit?

eteq · 2015-02-27T21:36:25Z

@timj - ok, yeah, that's true - just replace the bullet point that currently says "None" to briefly note this, and that it's planned for a future SOFA. Then we can merge this and I can run through a release.

timj · 2015-02-27T21:51:12Z

I've pushed the README tweak.

eteq · 2015-02-27T22:07:09Z

Ok, great, thanks @timj. Will merge this now, and go through the release sometime next week. Feel free to ping me if it hasn't happened...

Includes minor change to README (last commit in the merge)

sergiopasra · 2015-03-02T22:22:30Z

@timj sorry to arrive late to this. It seems sofa 20150209 has removed some functions: eraPmsafe and eraPvtob. I don't know what these functions were supposed to do, but removing them breaks backwards compatibility of the library: programs dynamically linked with previous liberfa will not work with this one unless recompiled from source.

If somebody is in contact with the SOFA people, please tell them that backwards compatibility is important.

timj · 2015-03-02T22:28:01Z

Are you sure they've gone? Their definitions moved around in the sofa.h file but they are still there.

sergiopasra · 2015-03-02T23:38:02Z

@timj Yes, they are there... How is that in the PR is a -int eraPmsafe but no a +int eraPmsafe? That's why I thought the function was removed

sergiopasra · 2015-03-02T23:39:18Z

Here, was this function signature duplicated?

timj · 2015-03-02T23:39:30Z

Because the function was defined twice...

sergiopasra · 2015-03-02T23:40:20Z

@timj oh, fine!

Update to SOFA 2015-02-09

92762f6

eteq reviewed Feb 25, 2015
View reviewed changes

timj added 2 commits February 26, 2015 10:17

Update README to indicate that there are no local patches

257c782

The compiler warning in dat.c was fixed in commit 506f302

Versions should be updated by release manager

5a9ddc4

Sync dat.c with corrected version from 2015-02-09 release

1a09a8f

The initial SOFA release had inadvertently removed a prior compiler warning fix.

Be explicit about the dat.c compiler warning problem

6376e54

eteq merged commit 6376e54 into liberfa:master Feb 27, 2015

eteq added a commit that referenced this pull request Feb 27, 2015

Merge pull request #26 from timj/feb15-sofa-rel

193fc9a

Includes minor change to README (last commit in the merge)

sergiopasra reviewed Mar 2, 2015
View reviewed changes

Comment thread src/erfa.h

sergiopasra Mar 2, 2015

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

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

Here, was this function signature duplicated?

This was referenced May 21, 2015

Upcoming leapsecond updates for older releases astropy/astropy#3791

Closed

Update local ERFA to include 2015-Jun-30 leap second astropy/astropy#3794

Merged

sergiopasra mentioned this pull request May 28, 2015

Please release erfa-1.2 #28

Closed

Conversation

timj commented Feb 25, 2015

Uh oh!

astrofrog commented Feb 25, 2015

Uh oh!

astrofrog commented Feb 25, 2015

Uh oh!

astrofrog commented Feb 25, 2015

Uh oh!

eteq commented Feb 25, 2015

Uh oh!

astrofrog commented Feb 25, 2015

Uh oh!

eteq Feb 25, 2015

Choose a reason for hiding this comment

Uh oh!

timj Feb 25, 2015

Choose a reason for hiding this comment

Uh oh!

eteq Feb 25, 2015

Choose a reason for hiding this comment

Uh oh!

mdboom Feb 25, 2015

Choose a reason for hiding this comment

Uh oh!

timj Feb 25, 2015

Choose a reason for hiding this comment

Uh oh!

eteq Feb 25, 2015

Choose a reason for hiding this comment

Uh oh!

eteq commented Feb 25, 2015

Uh oh!

eteq commented Feb 25, 2015

Uh oh!

timj commented Feb 25, 2015

Uh oh!

timj commented Feb 26, 2015

Uh oh!

eteq commented Feb 26, 2015

Uh oh!

scottransom commented Feb 26, 2015

Uh oh!

timj commented Feb 26, 2015

Uh oh!

timj commented Feb 27, 2015

Uh oh!

eteq commented Feb 27, 2015

Uh oh!

timj commented Feb 27, 2015

Uh oh!

eteq commented Feb 27, 2015

Uh oh!

timj commented Feb 27, 2015

Uh oh!

eteq commented Feb 27, 2015

Uh oh!

sergiopasra commented Mar 2, 2015

Uh oh!

timj commented Mar 2, 2015

Uh oh!

sergiopasra commented Mar 2, 2015

Uh oh!

sergiopasra Mar 2, 2015

Choose a reason for hiding this comment

Uh oh!

timj commented Mar 2, 2015

Uh oh!

sergiopasra commented Mar 2, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants