spacetelescope / jwst_gtvt

Other
10 stars 11 forks source link

Updates to Make GTVT python 3.7/3.8 Compatible. #56

Closed mfixstsci closed 3 years ago

mfixstsci commented 3 years ago
pep8speaks commented 3 years ago

Hello @mfixstsci! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 117:80: E501 line too long (111 > 79 characters) Line 118:80: E501 line too long (121 > 79 characters) Line 118:109: E231 missing whitespace after ',' Line 142:80: E501 line too long (116 > 79 characters) Line 142:105: E262 inline comment should start with '# ' Line 145:80: E501 line too long (80 > 79 characters) Line 146:80: E501 line too long (133 > 79 characters) Line 148:80: E501 line too long (129 > 79 characters) Line 163:80: E501 line too long (105 > 79 characters) Line 201:80: E501 line too long (119 > 79 characters) Line 202:13: E128 continuation line under-indented for visual indent Line 202:80: E501 line too long (107 > 79 characters) Line 377:80: E501 line too long (109 > 79 characters) Line 383:80: E501 line too long (132 > 79 characters) Line 560:80: E501 line too long (106 > 79 characters) Line 560:95: E262 inline comment should start with '# ' Line 564:80: E501 line too long (128 > 79 characters) Line 566:80: E501 line too long (124 > 79 characters) Line 580:80: E501 line too long (105 > 79 characters) Line 613:80: E501 line too long (119 > 79 characters) Line 614:13: E128 continuation line under-indented for visual indent Line 614:80: E501 line too long (107 > 79 characters) Line 808:80: E501 line too long (115 > 79 characters) Line 815:80: E501 line too long (138 > 79 characters)

Line 73:6: E131 continuation line unaligned for hanging indent

Comment last updated at 2021-07-07 19:03:18 UTC
mfixstsci commented 3 years ago

Interesting, looks like we have two travis build errors for python 3.6 and 3.7

This is okay, python 3.8 passes and the test is executed (what we were hoping to accomplish) so we should just move ahead with the review process. This PR should be rebased and merged after the tier two standards are implemented! (#55 )

mfixstsci commented 3 years ago

@PatrickOgle @bholler @JAStansberry I am thinking that once the tier two community software standards (#55) are merged, we should review and merge this and release a new version of the software.

JAStansberry commented 3 years ago

cc'ing Bryan


From: Mees Fix notifications@github.com Sent: Tuesday, February 9, 2021 1:44 PM To: spacetelescope/jwst_gtvt Cc: John Stansberry; Mention Subject: Re: [spacetelescope/jwst_gtvt] Updates to Make GTVT python 3.7/3.8 Compatible. (#56)

@PatrickOglehttps://github.com/PatrickOgle @bhollerhttps://github.com/bholler @JAStansberryhttps://github.com/JAStansberry I am thinking that once the tier two community software standards (#55https://github.com/spacetelescope/jwst_gtvt/pull/55) are merged, we should review and merge this and release a new version of the software.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/spacetelescope/jwst_gtvt/pull/56#issuecomment-776156242, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AGIEGPUEDWX4N4QXEEQSPPDS6F7CTANCNFSM4XJHRGEQ.

mfixstsci commented 3 years ago

In the ".travis.yml" file I see 3.6, 3.7, and 3.8 listed, but in "setup.py" it states that the version must be 3.7 or newer. Just wanted to note the inconsistency; if these two files would not clobber each other, then I think everything looks fine.

@bholler Ugh, okay the travis file is going away but I guess 43351b0 didn't resolve that. I am removing it by force now and we should be good to go. I see that the github action builds are passing, which is good. Do the code changes look okay? I had ot change some of the syntax for the date formatting etc.

bholler commented 3 years ago

@mfixstsci Looks like the code changes are limited to removing "out_subfmt='date'" where they are present. Were these changes required for newer versions of python? Are the dates still output correctly to the screen? Using a YYYY-MM-DD format for the outputs is preferred over an MJD format.

mfixstsci commented 3 years ago

@bholler here is what the dates will look like. The output format is no longer supported and I will say, this is not the prettiest. Maybe I can find a similar looking output to what is in the readme example output. It also looks like it shifts the table columns which is not good either.

$ jwst_gtvt 16:52:58.9 02:24:03

       Target
                ecliptic
RA      Dec     latitude
253.245   2.401  24.771

Checked interval [2020-01-01T00:00:00.000, 2023-12-31T00:00:00.000]
|           Window [days]                 |    Normal V3 PA [deg]    |
   Start           End         Duration         Start         End         RA            Dec     
 2020-02-24T22:38:46.969 2020-04-22T21:35:59.980       57.96     279.64411     249.47611     253.24542       2.40083 
 2020-07-12T21:36:00.152 2020-09-09T21:35:59.980       59.00     124.38425      94.74234     253.24542       2.40083 
 2021-02-24T21:36:00.152 2021-04-23T21:35:59.980       58.00     279.32715     248.90449     253.24542       2.40083 
 2021-07-12T21:36:00.152 2021-09-09T21:35:59.980       59.00     124.54229      94.87365     253.24542       2.40083 
 2022-02-24T21:36:00.152 2022-04-23T21:35:59.980       58.00     279.42431     249.15401     253.24542       2.40083 
 2022-07-13T21:36:00.152 2022-09-10T21:35:59.980       59.00     123.90127      94.59937     253.24542       2.40083 
 2023-02-24T21:36:00.152 2023-04-23T21:35:59.980       58.00     279.52495     249.40672     253.24542       2.40083 
 2023-07-13T21:36:00.152 2023-09-10T21:35:59.980       59.00     124.06733      94.73488     253.24542       2.40083 
mfixstsci commented 3 years ago

@bholler here is what the dates will look like. The output format is no longer supported and I will say, this is not the prettiest. Maybe I can find a similar looking output to what is in the readme example output. It also looks like it shifts the table columns which is not good either.

$ jwst_gtvt 16:52:58.9 02:24:03

       Target
                ecliptic
RA      Dec     latitude
253.245   2.401  24.771

Checked interval [2020-01-01T00:00:00.000, 2023-12-31T00:00:00.000]
|           Window [days]                 |    Normal V3 PA [deg]    |
   Start           End         Duration         Start         End         RA            Dec     
 2020-02-24T22:38:46.969 2020-04-22T21:35:59.980       57.96     279.64411     249.47611     253.24542       2.40083 
 2020-07-12T21:36:00.152 2020-09-09T21:35:59.980       59.00     124.38425      94.74234     253.24542       2.40083 
 2021-02-24T21:36:00.152 2021-04-23T21:35:59.980       58.00     279.32715     248.90449     253.24542       2.40083 
 2021-07-12T21:36:00.152 2021-09-09T21:35:59.980       59.00     124.54229      94.87365     253.24542       2.40083 
 2022-02-24T21:36:00.152 2022-04-23T21:35:59.980       58.00     279.42431     249.15401     253.24542       2.40083 
 2022-07-13T21:36:00.152 2022-09-10T21:35:59.980       59.00     123.90127      94.59937     253.24542       2.40083 
 2023-02-24T21:36:00.152 2023-04-23T21:35:59.980       58.00     279.52495     249.40672     253.24542       2.40083 
 2023-07-13T21:36:00.152 2023-09-10T21:35:59.980       59.00     124.06733      94.73488     253.24542       2.40083 

@bholler it turns that the 'date' subformat is support for over date times like utc and iso/isot. I found a solution that might mitigate a lot of code changes but will display the date as YY/MM/DD.

>>> Time('51179.00000143', format='mjd').datetime.strftime("%x")
01/01/99
bholler commented 3 years ago

If there were some way to exclude the time from the output, I think that would fix both issues: readability and columns being shifted. The time doesn't matter since it outputs values on each day by default. Additionally, if someone is using this tool to get precise information on target positions on timescales shorter than a day, then they are using it wrong.

mfixstsci commented 3 years ago

If there were some way to exclude the time from the output, I think that would fix both issues: readability and columns being shifted. The time doesn't matter since it outputs values on each day by default. Additionally, if someone is using this tool to get precise information on target positions on timescales shorter than a day, then they are using it wrong.

>>> Time('51179.00000143', format='mjd').datetime.date().strftime("%Y-%m-%d") '1999-01-01'

this seems to work okay!

mfixstsci commented 3 years ago

@bholler just made the change, here is what the output looks like:

$ jwst_gtvt 16:52:58.9 02:24:03

       Target
                ecliptic
RA      Dec     latitude
253.245   2.401  24.771

Checked interval [2020-01-01, 2023-12-31]
|           Window [days]                 |    Normal V3 PA [deg]    |
   Start           End         Duration         Start         End         RA            Dec     
 2020-02-24      2020-04-22        57.96     279.64411     249.47611     253.24542       2.40083 
 2020-07-12      2020-09-09        59.00     124.38425      94.74234     253.24542       2.40083 
 2021-02-24      2021-04-23        58.00     279.32715     248.90449     253.24542       2.40083 
 2021-07-12      2021-09-09        59.00     124.54229      94.87365     253.24542       2.40083 
 2022-02-24      2022-04-23        58.00     279.42431     249.15401     253.24542       2.40083 
 2022-07-13      2022-09-10        59.00     123.90127      94.59937     253.24542       2.40083 
 2023-02-24      2023-04-23        58.00     279.52495     249.40672     253.24542       2.40083 
 2023-07-13      2023-09-10        59.00     124.06733      94.73488     253.24542       2.40083 
bholler commented 3 years ago

That output looks perfect!

mfixstsci commented 3 years ago

@PatrickOgle let me know what you think and we can release a new version of the tool. These changes will extend our tool out to Python 3.9 and end support for python 3.6. The tool builds and runs with versions 3.7, 3.8 and 3.9.

https://github.com/spacetelescope/jwst_gtvt/actions/runs/943196406

We will probably want to take action on this sooner than later. Here is a table of supported python versions and important dates. Security fixes for 3.6 end this year

https://en.wikipedia.org/wiki/History_of_Python#Table_of_versions

PatrickOgle commented 3 years ago

@mfixstsci @bholler In principle this should be fine. Thanks for keeping me in the loop. I would rather not perform a formal review of the PR. The only thing I might check on for you is whether or not I can install the new versions of the tool with one of the later Python versions. I will be happy to try this if you send me explicit instructions.

mfixstsci commented 3 years ago

@mfixstsci @bholler In principle this should be fine. Thanks for keeping me in the loop. I would rather not perform a formal review of the PR. The only thing I might check on for you is whether or not I can install the new versions of the tool with one of the later Python versions. I will be happy to try this if you send me explicit instructions.

@PatrickOgle I would say to pull down this pr and install it and try to run it locally. These steps are assuming you don't have the repository cloned, and anaconda installed.


1. $ git clone git@github.com:spacetelescope/jwst_gtvt.git
2. $ cd jwst_gtvt
3. $ git remote add upstream git@github.com:spacetelescope/jwst_gtvt.git
4. $ git fetch upstream pull/56/head:update-python-version
5. $ git checkout update-python-version
6. $ conda create -n jwst-3.7 python=3.7
7. $ conda activate jwst-3.7
8. $ python setup.py install
9. $ jwst_gtvt 16:52:58.9 02:24:03

If you have the repo cloned, you can start at step 3 and if you want to try out python 3.8 perform a conda deactivate back into your base conda environment and then start at step 6 and change all the 3.7 instances to 3.8.

Let me know if you have any questions, I hope these steps cover all of the necessary steps to get this tested.

mfixstsci commented 3 years ago

@bholler can you give me a review and approve it? I will go ahead and merge this today.

mfixstsci commented 3 years ago

@PatrickOgle do you think this will warrant a new version release?

v0.1.2 ---> v0.2.0?

bholler commented 3 years ago

@mfixstsci This branch looks good! I checked out the MTVT as well and that looks good, too.

I provided my approval earlier in the PR, but reiterate it with this comment.

The only (minor) thing I can think of to change before performing a release would be to move the default start date later. It is currently 2020-01-01, which is now well in the past. Perhaps 2021-06-01?

I personally would like to see this as version 0.2.0.

PatrickOgle commented 3 years ago

Agreed it should be release 0.2.0.

mfixstsci commented 3 years ago

@mfixstsci This branch looks good! I checked out the MTVT as well and that looks good, too.

I provided my approval earlier in the PR, but reiterate it with this comment.

The only (minor) thing I can think of to change before performing a release would be to move the default start date later. It is currently 2020-01-01, which is now well in the past. Perhaps 2021-06-01?

I personally would like to see this as version 0.2.0.

Hey @bholler this makes total sense with the launch slip! Let me go ahead and take care of that.

mfixstsci commented 3 years ago

@bholler should we update the tool's ephemeris too? If so, we should probably focus on getting this update, then update the dates and ephemeris in a serparate PR and then perform the release.

bholler commented 3 years ago

There is nothing to change for the ephemeris, since it is the same one from April 2018, as shown below. So changing the default start time is the only thing that needs to be done.

Screen Shot 2021-07-07 at 1 46 39 PM
JAStansberry commented 3 years ago

I suspect most folks will be using this to plan observations post-commissioning, or for cycle-2 even, so a default start date of 2022-06-01 might be even better.


From: Mees Fix @.***> Sent: Wednesday, July 7, 2021 1:30 PM To: spacetelescope/jwst_gtvt Cc: John Stansberry; Mention Subject: Re: [spacetelescope/jwst_gtvt] Updates to Make GTVT python 3.7/3.8 Compatible. (#56)

@mfixstscihttps://urldefense.com/v3/__https://github.com/mfixstsci__;!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!liti1d2tckFiCrhsA-NEohu7jZSizkV3s5JeliLxGRQ8-MKS9MXTpBhvDslG7ys$ This branch looks good! I checked out the MTVT as well and that looks good, too.

I provided my approval earlier in the PR, but reiterate it with this comment.

The only (minor) thing I can think of to change before performing a release would be to move the default start date later. It is currently 2020-01-01, which is now well in the past. Perhaps 2021-06-01?

I personally would like to see this as version 0.2.0.

Hey @bhollerhttps://urldefense.com/v3/__https://github.com/bholler__;!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!liti1d2tckFiCrhsA-NEohu7jZSizkV3s5JeliLxGRQ8-MKS9MXTpBhv4DU7rvI$ this makes total sense with the launch slip! Let me go ahead and take care of that.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https://github.com/spacetelescope/jwst_gtvt/pull/56*issuecomment-875792418__;Iw!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!liti1d2tckFiCrhsA-NEohu7jZSizkV3s5JeliLxGRQ8-MKS9MXTpBhvxTqQpE8$, or unsubscribehttps://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AGIEGPWIWO5HOLZMW227N2DTWSFKJANCNFSM4XJHRGEQ__;!!CrWY41Z8OgsX0i-WU-0LuAcUu2o!liti1d2tckFiCrhsA-NEohu7jZSizkV3s5JeliLxGRQ8-MKS9MXTpBhvuFyo9jU$.

mfixstsci commented 3 years ago

@bholler @JAStansberry okay, since the change is minor I just made the update. Feel free to look at the last commit: 6cde030

mfixstsci commented 3 years ago

@bholler and @JAStansberry this is why we have unit tests!

Think I found all of the references from 01-01-2020 --> 06-01-2022.

cd4f23f