Use single date for agasc1p8 promotion #333
Conversation
|
@javierggt - see #332 (comment). We can make a new PR like this after the promotion and when we have a definitive transition date. I do not agree that a single guess (and it really just a guess) at the transition date can be merged and promoted to flight now. |
javierggt
force-pushed
the
single-agasc1p8-date
branch
2 times, most recently
from
aaf9eb0 to
4196c4a
Compare
javierggt
force-pushed
the
single-agasc1p8-date
branch
from
4196c4a to
9d952e6
Compare
|
@javierggt - please document your unit testing in the usual way. |
|
And go ahead and add |
| identification. | ||
| """ | ||
| with ( | ||
| conf.set_temp("cache_starcats", False), | ||
| conf.set_temp("date_start_agasc1p8_earliest", "1994:001"), | ||
| conf.set_temp("date_start_agasc1p8", "2003:001"), |
There was a problem hiding this comment.
I don't see the utility in moving the date start for agasc1p8 for this test anymore.
There was a problem hiding this comment.
This test just verifies that the change took place. In other words: by setting an early date, the test will use 1p8, when in reality it used 1p7, and the test will fail.
We can rewrite the test and make it better, but somewhere we still need to check that the switch works.
There was a problem hiding this comment.
Regarding "This test just verifies that the change took place." I don't see how the config set_temp to date_start_agasc1p8 for this test does that. I think, functionally, that is happening in
the other test, test_get_starcat_only_agasc1p8 .
There was a problem hiding this comment.
@jeanconn - since the requested time frame in the line below is just before 2003:001, that means this is explicitly using AGASC 1.7 (as the test name implies).
There was a problem hiding this comment.
Yes, but since the requested time frame is also before the standard config time for 1.8, the explicit set_temp doesn't show us anything.
There was a problem hiding this comment.
It shows us that the code is not doing something silly like adding one day before comparing to the transition time. This is similar to other testing we've done of using a value that is purposely close to a limit.
Yes, not the strongest test in the world but the code is correctly testing both cases (before and after the limit). I think it is all good!
|
"And go ahead and add PLR1730 to the ruff ignore list or # noqa them." or somebody approve and merge #336 first. |
I left that unchanged, because Jean added the corresponding changes in the other PR. I saw no point in cherry-picking four commits, or making one commit that will cause conflicts. |
Oops. OK understood. |
| identification. | ||
| """ | ||
| with ( | ||
| conf.set_temp("cache_starcats", False), | ||
| conf.set_temp("date_start_agasc1p8_earliest", "1994:001"), | ||
| conf.set_temp("date_start_agasc1p8", "2003:001"), |
There was a problem hiding this comment.
@jeanconn - since the requested time frame in the line below is just before 2003:001, that means this is explicitly using AGASC 1.7 (as the test name implies).
| ): | ||
| # Force AGASC 1.7 and show that star identification fails | ||
| # Force AGASC 1.8 and show that star identification fails |
There was a problem hiding this comment.
since we are going back and forth with this test, @taldcroft, did you see this change in the comment?
Just making sure I did not change it wrongly.
Description
This uses a single date for the AGASC 1p7-1p8 promotion. Note that this is based on the branch of #332. It leaves the logic to be able to change the AGASC catalog file when getting star catalogs.
The main reason for this is that, even though the promotion dates are configurable parameters, the result also depends on whether the 1p8 file is present in
$SKA/data/agascor not, which means the parameters are not enough to guarantee reproducible results. With that out the window, a single date is as good, with simpler code.This PR has the lint errors that are fixed in PR #336.
Interface impacts
Testing
Unit tests
Independent check of unit tests by [REVIEWER NAME]
Functional tests
No functional testing.