GlacioHack / xdem

Analysis of digital elevation models (DEMs)
https://xdem.readthedocs.io/
MIT License
147 stars 41 forks source link

[POC] refactoring tests #549

Open adebardo opened 3 months ago

adebardo commented 3 months ago

Context

The purpose of this ticket is primarily to allow the developers of demcompare to become familiar with the xdem tool in order to prepare for the refinement of future tasks, including, for example, dem_processings, classification, or statistics.

As part of the federation of DEM comparison tools xDEM and demcompare, we propose this initial ticket to refactor the unit tests and consolidate similar lines in a conftest file. This ticket primarily has an educational purpose. Indeed, mastering the unit test files will allow the main developers of the federation to better understand the architecture of xDEM while offering an initial AGILE implementation.

Tasks

For the "coreg" folder

test_affine.py

test_base

test_biascorr

Estimate 3d

adebardo commented 3 months ago

@adehecq @duboise-cnes If you agreed for the coreg folder, i will complete this issue for the entire tests folder

duboise-cnes commented 3 months ago

Ok for the principle to begin on xdem tests to "feel" the code and be able to understand better xdem for integrating demcompare-like cli. Be aware to think how we can plug coreg, dem_processing, classif, stats (maybe more important) functional parts of a demcompare like CLI during this educational step. This purpose could be added to the issue explicitely.

I let @adehecq say if ok with the approach of refactoring to xdem. For my point of view Don't try to be exactly like demcompare. Interesting to take inspiration from pandora2d too which is newer as pytest organization. Helpers can be named another way. Maybe also important to converge on a test structure more clearly described (schema, simple refactoring architecture from existing functions) with amaury in the issue before coding ? Also use pytest fixtures is the key of factorization i think (not only conftest or helpers like module to aggregate functions), I would have put it explicitely also in description....

My 2cts for this issue.

rhugonnet commented 3 months ago

I let @adehecq complete if necessary, but I think I can say that this refactoring also makes great sense to us! :slightly_smiling_face: Same as @duboise-cnes noted, we had in mind to use pytest fixtures to make the test data loading more consistent than the current redundant implementation, see the issue we opened here and the references we noted especially for this purpose: https://github.com/GlacioHack/xdem/issues/427.

But we're really not experts in how best to do this, we have fairly limited experience and are open to a new structuring.

We have a bit of experience populating directly conftest.py for data download and preparation in other projects. For some reason in xdem we chose early to separate the data download and preparation methods into a separate examples.py module :thinking: (not necessarily the best name, was more oriented towards documentation than test originally). I think the reason might have been to avoid making conftest.py too large a file (@erikmannerfelt can probably comment on that).

adehecq commented 3 months ago

I also agree that some improvements can be made in the tests by using conftest.py and fixtures. I'm totally up for restructuring as needed. It's just a bit difficult for me to understand what it involves without a good example, so maybe the best is to do this restructuring for one submodule first and iterate on it before applying the same to other tests? Or describe a more detailed example of changes in this issue, if the first option is too long.

adebardo commented 3 months ago

Hello,

I can see that this approach to learning how to use xdem also addresses a need.

As @adehecq suggested, I can propose a first branch with the modifications related to the coregistration class. This will allow us to understand the use of pytest within the 3D tools and iterate on the implemented methods.

A few responses to your feedback: