Adding raster compression option

[Antsalacia]

Added a compression option in set_query_parameters and put new key in the output_metadata dictionary.
Method get_raster_compression added with a try and except.

[wehrad]

Thanks for working on this @Antsalacia! You create a get_raster_compression() method but never use it.

I guess you'd want this:

https://github.com/AdrienWehrle/earthspy/blob/71b3cc5afd7690a417420e1b048b99ced220c663/earthspy/earthspy.py#L178

to rather be something like:

self.get_raster_compression()

with self.compression being set inside the method, just like e.g. :

https://github.com/AdrienWehrle/earthspy/blob/71b3cc5afd7690a417420e1b048b99ced220c663/earthspy/earthspy.py#L204-L219

[Antsalacia]

Solved #84

[wehrad]

For now this code isn't working, but it's not visible since we can't run earthspy properly in tests at the moment (I'm working on getting credentials for testing purposes).

That's because in get_raster_compression() you're calling a compress_mode which is not passed as an argument to the function. So you need to pass compression_mode in the function just like evaluation_script is passed in self.get_evaluation_script(evaluation_script) right below.

Actually compress_mode doesn't exist anywhere else. You have a compression variable in set_query_parameters but compress_mode doesn't exist anywhere else than in the function.

Remember to run your modifications before pushing your code! If it doesn't work, please make a note that it will be fixed later! :)

[wehrad]

Also, could you:

• rename compression for raster_compression
• add documentation for the new raster_compression argument in the docstring of set_query_parameters
• add a few tests for the new raster_compression in test_earthspy.py

[Antsalacia]

What kind of test should I add in test_earthspy for raster_compression ?

[wehrad]

Have you tested if the raster saving works fine with "compress" to None in the metadata? You could try on the earthspy example in the README.

[Antsalacia]

The raster saving works fine when no argument is pass to raster_compress and thus equal to None

[wehrad]

The raster saving works fine when no argument is pass to raster_compress and thus equal to None

Cool thanks!

Then what remains is to resolve the discussion with CodeQL further up here and then you'd need to run the pre-commit hooks and pytest.

See CONTRIBUTING.md for details. :)

[wehrad]

I just added a commit to address CodeQL alert that raster_compression == None should be raster_compression is None.

I see you fixed pre-commit issues, do you still have any or does everything pass? Is pytest also passing all tests?

[wehrad]

Don't hesitate to git add all the files modified by black once you've run pre-commit. As you can see here black would like to format the code but you haven't pushed the results.

One would usually run pre-commit run --al-files twice, first to format, then to make sure formatting fixed all issues.

[wehrad]

closes #84

[wehrad]

Now that I think about it, we could only update the metadata dictionary to add the compression when raster_compression is not none, so that it doesn't unnecessarily write compression to None.

[wehrad]

I just added more comments and fixed a couple more things.

So I think the only thing missing here is to get test_get_raster_compression ton run on t3 and t4!

[Antsalacia]

How can I check raster_compression when I try this it doesn't work:
comp_def = t4.get_raster_compression()
error:
AttributeError: 'function' object has no attribute 'get_raster_compression'

[wehrad]

Maybe earthspy hasn't been reloaded. Can you try pip install -e . and then again?

[Antsalacia]

Still the same after pip

[wehrad]

Can you share where you're running this t4? Can you try on a simple example too where you just setup a job and then try access the function?

[Antsalacia]

I am running it in the test_get_raster_compression and t4 is in conftest. It works with the job.

[Antsalacia]

All good now was just testing the wrong way thanks for helping.

[wehrad]

Nice to hear! Now please modify the tests to compliy with github-advanced-security

[wehrad]

there's a typo left :) "sith"