-
Notifications
You must be signed in to change notification settings - Fork 9
Updates 2025 #91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Updates 2025 #91
Conversation
|
It passed! Thank you so much, @oberonia78 ! |
| yamale | ||
| backoff | ||
| isce3==0.15.0 | ||
| isce3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to specify the isce3 version. By the way, would you let me know what isce3 version has the updated and fixed function for RTC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. The RTC fix was merged to ISCE3 in (v0.24.4). So, I we probably can set it to isce3>=0.24.4. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer isce3 == 0.24.4, in case that isce3 may have some changes, which is not compatible with the current RTC workflow. By the way, I've not worked on 0.24.4 so far, and only tested with 0.24.3. Let me check if this version works for the RTC as well. Or have you already tested it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, @oberonia78 . Since this line sets the isce3 version for the Docker, I think it's fine. Will update it. Thanks! In regards to testing, I tested this PR against the ISCE3 develop branch, but I learned recently that v0.24.4 is being created from a different fork. So, I should retest it with v0.24.4. Thanks for checking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi both, I've noticed the isce3 versions haven't been bumped in the envrionment.yaml or the lockfile.lock. From what I can see the docker image references the lockfile, so on my build with gshiroma:updates_2025 I am still seeing isce3==0.15.0. Do these also need to be updated?
Apologies if I have missed something.
Quick test:
git clone --branch updates_2025 https://github.com/gshiroma/RTC.git
cd RTC
docker build -t rtc:updates_2025 -f Docker/Dockerfile .
docker run -it rtc:updates_2025 /bin/bash
> conda activate RTC && conda list
>>> isce3 0.15.0 py39h431996e_0 conda-forgeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @abradley60 ! @oberonia78 is working on that. He's been implementing these updates in PR #93. However, we're continuing the discussion offline, and he may end up creating two branches: one that supports the latest ISCE3 version, likely to remain the main branch, and another that stays frozen on a specific ISCE3 version to be used in production. The production branch may not include the RTC delay fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great thanks @gshiroma, clear to me.
oberonia78
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I think overall looks good. Thanks @gshiroma
|
@gshiroma and @oberonia78 there is big jump from ISCE3 v0.15 to 0.24.4. While syncing with ISCE3 recent version will be nice, I do not understand why this would be needed for making this repo to work for S1C. Can you guys elaborate? |
@hfattahi , the main goal of this PR is to fix the delay LUTs that are not being passed from the GeocodeCov to the RTC module. So, the images are expected to be different. We discussed this offline. |
I understand. That is definitely a welcoming change from my perspective. However, it should have been very clearly mentioned in this PR how that will impact the products. And what are other changes in core ISCE3 that impact RTC-S1. I don't see documentation of any of those changes. Ideally I suggest to decouple S1C support from ISCE3 core module improvements. OPERA production should have a chance to process S1C without taking all ISCE3 updates. |
Thank you, @hfattahi , I updated the description accordingly. Please, let me know if I missed anything. FWIW, OPERA DIST-S1 team is interested in this update and they are helping us testing this PR. |
Thanks for updating the description. Again the fixes are great and fine with me. I just wanted to make it clear that with this PR, the OPERA RTC-S1 products will slightly change (actually improved in my opinion) but that means that if this version will be used for forward production, the product version of RTC-S1 should be bumped up. This software will not reproduce the existing RTC-S1 products. |
|
Hi @hfattahi , sure, all great points. I appreciate you taking the time to check this. |
Updates to the RTC Software:
The ISCE3 version 0.24.4 includes a fix that extends the use of bistatic and dry-tropospheric delays from the geocoding (GeocodeCov) module to the RTC module (commit isce-framework/isce3@f69227d).
The new ISCE3 version also reduces the number of NaNs in the image by reducing the minimum ratio of valid samples from 75% to 0% (PR isce-framework/isce3@26e6728).
numpy.string_withnumpy.bytes_.v1.0.4.