Skip to content
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

Add in a lookuptable thumbnail function #105

Merged
merged 10 commits into from
Oct 26, 2020
Merged

Add in a lookuptable thumbnail function #105

merged 10 commits into from
Oct 26, 2020

Conversation

alexgleith
Copy link
Contributor

Add in a way to call the existing function for a singleband lookuptable from the dataset assembler class.

omad
omad previously approved these changes Oct 26, 2020
Copy link
Member

@omad omad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like an improvement to me, pushing more code in here from datacube-alchemist.

My only concern, and not a blocker, is that this doesn't go far enough. It's still too method call driven which means extra logic in the client. Maybe we should add parameters to DatasetAssembler.write_thumbnail() and it gets to decide on the method to use to generate the thumbnail...

@alexgleith
Copy link
Contributor Author

What do you think, @jeremyh? Want me to weave the two functions into a single meta function with two code paths?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 87.045% when pulling 4045426 on add-lut-thumbnail into 79425c9 on eodatasets3.

omad
omad previously approved these changes Oct 26, 2020
Copy link
Collaborator

@jeremyh jeremyh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. It's different enough of a situation that I don't mind a second function for convenience. Though I like where Damien's coming from.

If combining them means making all the current red/green/blue args optional on the existing function I'd probably prefer not to combine, as their arguments are too different and we'll have a confusing set of completely-mutually-exclusive optional/required parameter combinations on the one function.


Perhaps also add a line to one of our minimal tests to run the function, eg:

p.write_thumbnail_singleband('blue', None, bit=1)

to https://github.com/GeoscienceAustralia/eo-datasets/blob/224f97410995e083982fa20ce75fe8e069f8fdbf/tests/integration/test_assemble.py#L200

(I also ran it at first without any bit argument and got a cryptic error message -- we're never checking the bit is None and lookup_table is None case in our old method parameter checking)

eodatasets3/assemble.py Outdated Show resolved Hide resolved
eodatasets3/assemble.py Outdated Show resolved Hide resolved
@jeremyh
Copy link
Collaborator

jeremyh commented Oct 26, 2020

Bike shedding... should it be write_thumbnail_singleband or write_thumbnail_single_band to have consistent naming style? (we have, for example self.write_measurements_odc_xarray() elsewhere)

@alexgleith
Copy link
Contributor Author

Bike shedding... should it be write_thumbnail_singleband or write_thumbnail_single_band to have consistent naming style? (we have, for example self.write_measurements_odc_xarray() elsewhere)

I think singleband is a single word... keeping it as it is ;-)

@omad
Copy link
Member

omad commented Oct 26, 2020

Install pre-commit Alex!

Copy link
Member

@omad omad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👯

Copy link
Collaborator

@jeremyh jeremyh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@omad omad merged commit 26de464 into eodatasets3 Oct 26, 2020
@omad omad deleted the add-lut-thumbnail branch October 26, 2020 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants