Add 'day_night' flag to DayNightCompositor for day-only or night-only results #1816

yukaribbba · 2021-09-02T14:58:31Z

As discussed in #1780 , I want to make some enhancements on DayNightCompositor so that we can chose which side (day/night or both) we want to keep in the final output image. Thank @djhoese and @pnuu for their suggestions.

Closes What shoud I do if I only want to keep the day part of DayNightCompositor? #1780
Tests added
Fully documented
Add your name to AUTHORS.md if not there already

djhoese · 2021-09-02T15:04:38Z

satpy/composites/__init__.py

        """Collect custom configuration values.

        Args:
            lim_low (float): lower limit of Sun zenith angle for the
                             blending of the given channels
            lim_high (float): upper limit of Sun zenith angle for the
                             blending of the given channels
+            chose_day_night (string): "day_and_night" means both day and night parts will be kept


What do you think about day_night for the keyword argument (otherwise is should be choose_ not chose_)?

Sorry for the spell...

djhoese · 2021-09-02T15:05:31Z

satpy/composites/__init__.py

        super(DayNightCompositor, self).__init__(name, **kwargs)

    def __call__(self, projectables, **kwargs):
        """Generate the composite."""
        projectables = self.match_data_arrays(projectables)

-        day_data = projectables[0]
-        night_data = projectables[1]
+        data_1 = projectables[0]


Is there a more descriptive name for this? I suppose "foreground" isn't really correct here.

I think you are right. Whatever you choose, you need a 'foreground' image at least. So 'foreground' and 'background' are more suitable.

djhoese · 2021-09-02T15:06:08Z

satpy/composites/__init__.py

@@ -585,43 +588,99 @@ def __call__(self, projectables, **kwargs):
            LOG.debug("Computing sun zenith angles.")
            # Get chunking that matches the data
            try:
-                chunks = day_data.sel(bands=day_data['bands'][0]).chunks
+                chunks = data_1.sel(bands=data_1['bands'][0]).chunks


Github won't let me comment on it, but the coszen = line above uses projectables[2] which may or may not exist depending on the day/night mode.

This projectables[2] remains in the original one. I actually don't know what it's for...May be it's waiting for a solar zenith angle band/dataset from the satellite data?

🤦 Oh duh. That is if the YAML file specifies the solar_zenith_angle dataset from the reader (ex. for viirs_sdr). Geostationary satellites don't typically have that provided by the file/reader so it isn't specified. You may want this to be projectables[2 if self.day_night == "day_night" else 1].

😊OK It's late night here so I'll do these changes tomorrow.

djhoese · 2021-09-02T15:17:56Z

satpy/composites/__init__.py

-        # Blend the two images together
-        data = (1 - coszen) * night_data + coszen * day_data
-        data.attrs = attrs
+        elif self.chose_day_night == "only_night":


Can the day/night only modes be combined with if statements for the things that are different? You could also ignore the 0 parameters in the logic so that extra computation isn't done when it isn't needed. Oh you might be able to combine all three day/night modes...

day_data = projectables[0] if "day" in self.day_night else None night_data = projectables[0] if self.day_night == "night_only" else None night_data = projectables[1] if self.day_night == "day_night" else night_data day_portion = coszen * (day_data if day_data is not None else 0) night_portion = (1 - coszen) * (night_data if night_data is not None else 0) data = night_portion + day_portion

You'd need some more logic for handling the add_bands and zero_missing_data stuff, but it may actually make the code a lot smaller. Maybe not clearer though... 😨

Yeah I also think there's something need to be improved in the logic structure. I'll go look into this.

satpy/composites/__init__.py

yukaribbba · 2021-09-03T14:59:16Z

I've made some fixes on the variable names and the whole IF structure.

djhoese · 2021-09-09T15:32:17Z

😕 How did you get appveyor to start running again? 😕

yukaribbba · 2021-09-09T15:46:48Z

Oh I'm still waiting if there are more changes need to be done then I'll get the tests and documents.

djhoese · 2021-09-09T15:49:04Z

Sorry, what I mean is that "appveyor" is one of the checks being run. We haven't used appveyor for years so it is very strange that it is even trying to run.

That said, you will need to merge with upstream main to fix the conflicts so we can do a proper review. Let me know if you need help figuring that out.

yukaribbba · 2021-09-09T15:57:44Z

Yeah...This's my first PR so I don't know how it happened. I do have a conflict warning saying "Only those with write access to this repository can merge pull requests." How can I resolve it?

djhoese · 2021-09-09T16:02:00Z

I do have a conflict warning saying "Only those with write access to this repository can merge pull requests." How can I resolve it?

That is not a warning about the conflict. That is message telling you that you can't merge your pull request into upstream Satpy. Only the Satpy maintainers/core developers have permissions to do that.

You should see a message saying "Conflicting files"...nevermind, I checked and the conflicts were fairly simple. I just fixed them with GitHub's editor. You should make sure to run git pull on your local machine to get the changes I made before doing anything else. There are no longer conflicting files so once the tests pass we can review this again. Thanks.

yukaribbba · 2021-09-09T16:07:47Z

Thanks but I'm still confused. How this happened and how to avoid it in the future? I'm using GitHub Desktop.

djhoese · 2021-09-09T16:22:08Z

There are two things that happened:

For some reason appveyor or GitHub decided to try to run appveyor tests. I have no idea why this happened. This was out of your control and had nothing to do with what you did (I think). If anything, it may have been that appveyor is still looking at this repository and even though we don't use appveyor it was complaining about their being conflicts. Ignore this.
Regarding the conflicts: This is a common occurrence when there is any actually activity on a repository. Let's say you and I started working on two separate pull requests. We both create new git branches to hold our work based off of the upstream main branch which may be at commit A (as an example). If I make a commit and push to my branch for my pull request then my branch is at commit A + 1, let's call it A + x. If you make a commit and push to your branch then your branch is at A + 1, but still a different commit than my branch, let's call it A + y. The point is that our branches are one commit ahead of the main branch. Now, say that I finish my pull request and it gets merged before yours. The upstream main branch is now at commit A + x. So now git and github have to decide: is it possible to combine A + y (your branch) with A + x (the main branch). If we didn't edit the same files then it is obvious, just make your changed files the new versions of the files. But if we both edited the same files then git/github don't know how to do that. So instead of saying everything is fine and the branches can be merged it will say there are conflicts and that you, the author, need to figure out how to resolve these conflicts yourself. You can do this by either fetching/pulling the various branches to your local machine and doing a git merge main and resolving the conflicts or you can click "Resolve Conflicts" on the GitHub page and try to edit the files in the basic text editor that GitHub gives you (fine for simple changes, not fine for anything complex).

Probably more info that you wanted, but here we are.

yukaribbba · 2021-09-09T16:44:19Z

Ok I got it. Thanks a lot for these details. That's exactly what I need.😁

This reverts commit 630baaf.

This reverts commit 80ed09a, reversing changes made to 2ceb218.

This reverts commit baf8501.

yukaribbba · 2021-09-10T12:54:28Z

@djhoese Current tests are just for "day_night". Do you think we need more other ones for "only_day" or “only_night”?

djhoese

I had a few small changes and questions. As for the tests, I think you need to have them for both day_only and night_only too. Lastly, why did the test_add_bands_l_rgb test have to change to get A added? That sounds like a bug. The existing behavior should not have changed.

djhoese · 2021-09-10T13:28:30Z

satpy/composites/__init__.py

@@ -551,69 +551,79 @@ def _insert_palette_colors(channels, palette):
 class DayNightCompositor(GenericCompositor):
    """A compositor that blends a day data with night data."""

-    def __init__(self, name, lim_low=85., lim_high=88., **kwargs):
+    def __init__(self, name, lim_low=85., lim_high=88., day_night="day_only", **kwargs):


Shouldn't the default be day_night? And could you mention that it is the default in the docstring below. Something simple like (default) next to the name.

djhoese · 2021-09-10T13:30:27Z

satpy/composites/__init__.py

@@ -678,7 +688,7 @@ def add_bands(data, bands):
        data['bands'] = new_bands
        data.attrs['mode'] = mode
    # Add alpha band
-    if 'A' not in data['bands'].data and 'A' in bands.data:
+    if ('A' not in data['bands'].data and 'A' in bands.data) or ('A' not in data['bands'].data):


Unless I'm reading this wrong, is the first expression the same as the last one? If so then logically this just becomes if 'A' not in data['bands'].data: which I don't think is what is wanted, right?

Oops that's for the previous structure. I'll fix it.

yukaribbba · 2021-09-10T13:54:20Z

I change the test_add_bands_l_rgb because I got an error in pytest. New add_bands will add an alpha chanel to L/RGB images. So maybe we'll expect a different result.

djhoese · 2021-09-13T18:52:38Z

satpy/composites/__init__.py

-        day_data = projectables[0]
-        night_data = projectables[1]
+        # At least one composite is requested.
+        foreground_data = projectables[0]


match_data_arrays should always be called. You can do projectables = self.match_data_arrays(projectables) just above this line. match_data_arrays checks a lot of various things and modifies the provided DataArrays as needed to make them compatible with the compositor or with the rest of Satpy.

Well when I tested true_color_with_night_ir_alpha I got errors saying Dimension 'bands' size mismatch: 3 != 4 with match_data_arrays. This even occurred under original version. I didn't know what happened...

Is this something related with #1821 and #1813 ?

Yes, this is exactly #1821. I'm trying to finish #1815 and then fix #1821. Everything is just taking longer than I expected. One work around is to switch to an older version of xarray (<0.19.0 if I remember correctly).

Ok so I'll just put match_data_arrays back here.

djhoese · 2021-09-16T16:07:15Z

@yukaribbba The PRs we mentioned before have been merged to main. Please try merging with the pytroll upstream main branch and see how things run for you.

…positor

djhoese · 2021-09-16T16:26:10Z

Flake8 issues from CI:

satpy/composites/__init__.py:555:1: D205 1 blank line required between summary line and description
satpy/composites/__init__.py:663:1: D205 1 blank line required between summary line and description

yukaribbba · 2021-09-16T16:29:09Z

@djhoese OK got it. But there're some errors running my script.

Traceback (most recent call last):
  File "C:/Users/45107/Downloads/Sat/GK2A/blue.py", line 2, in <module>
    from satpy import Scene, find_files_and_readers
  File "c:\users\45107\documents\github\satpy\satpy\__init__.py", line 21, in <module>
    from satpy.version import version as __version__  # noqa
ModuleNotFoundError: No module named 'satpy.version'

djhoese · 2021-09-16T16:34:57Z

You'll need to reinstall the library, so pip install -e . --no-deps should do it. When Satpy is installed it creates that version module.

yukaribbba · 2021-09-17T09:23:44Z

@djhoese Several combinations tested, and no problems there. So I'm moving forward to documents.

mraspaud

LGTM

mraspaud · 2021-09-17T11:37:08Z

@yukaribbba is this ready for merging?

yukaribbba · 2021-09-17T12:58:46Z

@mraspaud I just updated the documentation for review. After that, I think it would be ready.

mraspaud · 2021-09-17T13:01:57Z

perfect, I'm looking now

mraspaud

LGTM

mraspaud · 2021-09-17T13:07:51Z

@djhoese are you good with the PR?

djhoese

Looks good to me! Thanks for all the work @yukaribbba. I'll merge this when the tests finish and pass.

doc/source/composites.rst

yukaribbba · 2021-09-17T13:36:46Z

Great! That's my first PR. Thank you @djhoese @mraspaud for all the helps!

… results (pytroll#1816) * Update __init__.py * Update __init__.py * Update __init__.py * Update __init__.py * tests * Revert "tests" This reverts commit 630baaf. * Revert "Merge branch 'main' into feature-daynightcompositor" This reverts commit 80ed09a, reversing changes made to 2ceb218. * Revert "Revert "Merge branch 'main' into feature-daynightcompositor"" This reverts commit baf8501. * update add_bands_test * update DayNightCompositor test * update DayNightCompositor test * new structure * Update __init__.py * Update __init__.py * Update __init__.py * Update __init__.py * Update __init__.py * Update __init__.py * Update test_composites.py * Update satpy/composites/__init__.py Co-authored-by: David Hoese <david.hoese@ssec.wisc.edu> * Update satpy/composites/__init__.py * Update __init__.py * Update __init__.py * Update __init__.py * Update __init__.py * Update __init__.py * Update __init__.py * Update __init__.py * Update __init__.py * Update __init__.py * Update composites.rst * Update AUTHORS.md * Update doc/source/composites.rst Co-authored-by: David Hoese <david.hoese@ssec.wisc.edu>

Update __init__.py

9cce883

yukaribbba requested review from djhoese and mraspaud as code owners September 2, 2021 14:58

yukaribbba changed the title ~~Update __init__.py~~ Improvements on DayNightCompositor Sep 2, 2021

djhoese requested changes Sep 2, 2021

View reviewed changes

Update __init__.py

4a75db1

stickler-ci reviewed Sep 3, 2021

View reviewed changes

satpy/composites/__init__.py Outdated Show resolved Hide resolved

yukaribbba added 2 commits September 3, 2021 23:23

Update __init__.py

b3b7b35

Update __init__.py

2ceb218

yukaribbba requested a review from djhoese September 9, 2021 14:52

Merge branch 'main' into feature-daynightcompositor

80ed09a

yukaribbba added 7 commits September 10, 2021 18:19

tests

630baaf

Revert "tests"

b294981

This reverts commit 630baaf.

Revert "Merge branch 'main' into feature-daynightcompositor"

baf8501

This reverts commit 80ed09a, reversing changes made to 2ceb218.

Revert "Revert "Merge branch 'main' into feature-daynightcompositor""

6bcf7eb

This reverts commit baf8501.

update add_bands_test

82b84a6

update DayNightCompositor test

a700799

update DayNightCompositor test

9bf4207

djhoese requested changes Sep 10, 2021

View reviewed changes

yukaribbba added 2 commits September 14, 2021 02:13

Update __init__.py

cd55547

Update __init__.py

8d8377a

djhoese reviewed Sep 13, 2021

View reviewed changes

Update __init__.py

af2c6ee

This was referenced Sep 15, 2021

Add MODIS L2 products produced by IMAPP #1815

Merged

Fix unify_chunks usage in compositors and fix image mode in BackgroundCompositor #1823

Merged

Merge remote-tracking branch 'upstream/main' into feature-daynightcom…

787a6a9

…positor

Update __init__.py

0a75d4a

mraspaud approved these changes Sep 17, 2021

View reviewed changes

Update composites.rst

b59557f

yukaribbba requested a review from pnuu as a code owner September 17, 2021 12:58

mraspaud approved these changes Sep 17, 2021

View reviewed changes

Update AUTHORS.md

9922f23

djhoese approved these changes Sep 17, 2021

View reviewed changes

djhoese reviewed Sep 17, 2021

View reviewed changes

doc/source/composites.rst Outdated Show resolved Hide resolved

Update doc/source/composites.rst

872d95a

djhoese changed the title ~~Improvements on DayNightCompositor~~ Add 'day_night' flag to DayNightCompositor for day-only or night-only results Sep 17, 2021

djhoese merged commit 9d7cca2 into pytroll:main Sep 17, 2021

yukaribbba deleted the feature-daynightcompositor branch September 17, 2021 14:43

Add 'day_night' flag to DayNightCompositor for day-only or night-only results #1816

Add 'day_night' flag to DayNightCompositor for day-only or night-only results #1816

Conversation

yukaribbba commented Sep 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yukaribbba commented Sep 3, 2021

djhoese commented Sep 9, 2021

yukaribbba commented Sep 9, 2021

djhoese commented Sep 9, 2021

yukaribbba commented Sep 9, 2021

djhoese commented Sep 9, 2021

yukaribbba commented Sep 9, 2021

djhoese commented Sep 9, 2021

yukaribbba commented Sep 9, 2021

yukaribbba commented Sep 10, 2021

djhoese left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yukaribbba commented Sep 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djhoese commented Sep 16, 2021

djhoese commented Sep 16, 2021

yukaribbba commented Sep 16, 2021

djhoese commented Sep 16, 2021

yukaribbba commented Sep 17, 2021

mraspaud left a comment

Choose a reason for hiding this comment

mraspaud commented Sep 17, 2021

yukaribbba commented Sep 17, 2021

mraspaud commented Sep 17, 2021

mraspaud left a comment

Choose a reason for hiding this comment

mraspaud commented Sep 17, 2021

djhoese left a comment • edited

Choose a reason for hiding this comment

yukaribbba commented Sep 17, 2021

yukaribbba commented Sep 2, 2021 •

edited

djhoese left a comment •

edited