opera-adt / COMPASS

COregistered Multi-temPorAl Sar Slc
Apache License 2.0
39 stars 18 forks source link

Unit test for IONEX functionalities #80

Closed vbrancat closed 1 year ago

vbrancat commented 1 year ago

This PR addresses issue #61 and adds the unit test for testing the IONEX reader and interpolation methods.

At the moment, the PR includes the IONEX golden dataset inside the repository. Eventually, the unit test can include functionality to download the required data for the test.

Not sure how to set up the test to run on the CI system

scottstanie commented 1 year ago

A separate note- The way the Dockerfile is currently set up isn't great for testing our own code changes. The 4th command (line 11) is COPY . /home/compass_user/OPERA/COMPASS Then after that, all of the installing and setup happens. So if you change any file, it invalidates the cache and makes you run everything after that. Since code changes are more ocmmon than environment/requirement changes the COPY should go as late as possible @seongsujeong

vbrancat commented 1 year ago

A separate note- The way the Dockerfile is currently set up isn't great for testing our own code changes. The 4th command (line 11) is COPY . /home/compass_user/OPERA/COMPASS Then after that, all of the installing and setup happens. So if you change any file, it invalidates the cache and makes you run everything after that. Since code changes are more ocmmon than environment/requirement changes the COPY should go as late as possible @seongsujeong

@seongsujeong any opinion on this?

LiangJYu commented 1 year ago

The unit test only works for scipy < 1.10. Do we want use scipy > 1.10 and debug/refactor compass.utils.iono? Or force scipy the environment scipy <= 1.9?

LiangJYu commented 1 year ago

The unit test only works for scipy < 1.10. Do we want use scipy > 1.10 and debug/refactor compass.utils.iono? Or force scipy the environment scipy <= 1.9?

Disregard above. Fixed with e0f2b02

vbrancat commented 1 year ago

@scottstanie @LiangJYu ready for you to review whenever you have time. All your comments have been addressed.

LiangJYu commented 1 year ago

A separate note- The way the Dockerfile is currently set up isn't great for testing our own code changes. The 4th command (line 11) is COPY . /home/compass_user/OPERA/COMPASS Then after that, all of the installing and setup happens. So if you change any file, it invalidates the cache and makes you run everything after that. Since code changes are more ocmmon than environment/requirement changes the COPY should go as late as possible @seongsujeong

fixed?

LiangJYu commented 1 year ago

@seongsujeong @scottstanie Can you guys please take a look? I think the pressing issues in the PR have been addressed. Thanks.

seongsujeong commented 1 year ago

A separate note- The way the Dockerfile is currently set up isn't great for testing our own code changes. The 4th command (line 11) is COPY . /home/compass_user/OPERA/COMPASS Then after that, all of the installing and setup happens. So if you change any file, it invalidates the cache and makes you run everything after that. Since code changes are more ocmmon than environment/requirement changes the COPY should go as late as possible @seongsujeong

I agree that can be an efficient way of building Docker file, but I think I'd prefer to invalidate the cache when we make any changes to the file we are copying into. That would make docker build slower, but we can make sure any changes in the code are properly applied into the layers of the Docker images. Note that we are using the same Dockerfile not only for test but also release.

LiangJYu commented 1 year ago

I'm not sure if anything can be done to address remaining codacy issues. If anyone has fixes, please share!

seongsujeong commented 1 year ago

I'm not sure if anything can be done to address remaining codacy issues. If anyone has fixes, please share!

I see codacy issue on the line 46 in Dockerfile. Can you try just removing the echo commands that prints out the messages into stdout?

The line is just to show the progress, but those messages are hard to read anyhow because the Docker build process gives out tons of lines of messages.