Up to date

[godinlu]

• Simplified the file organization
• Added .gitignore rules for the notebooks
• Updated the tools file, as it was outdated

[friedrichknuth]

Thank you so much for the PR @godinlu !! 🙌

I've added a few minor questions and comments below, which should be easily addressed/resolved before we merge this 💪

[friedrichknuth]

Are these changes necessary for the new re-organization? I think it should still work like this, even after removing the sub-folder structure?

Import hipp.module seems cleaner than from . import module, but I'm not sure if one is better than the other.

In a more recent library I don't have the sub-folder structure and am importing the modules in the __init__.py with import gtsa.module for example.

[godinlu]

Both approaches work fine — the main difference is that import hipp.module uses an absolute import, while from . import module is a relative one. The relative form is a bit more robust in case the package name ever changes, since it doesn’t depend on the package being named hipp.

That said, I’m happy to switch to absolute imports if you prefer.

[friedrichknuth]

OK makes sense and thank you for clarifying. I think it is OK to be explicit here about importing the modules from the library called hipp. My preference is to use import hipp.module.

[adehecq]

My preference also goes to absolut imports. Actually, in xdem we have a pre-commit linter called absolufy-imports.

[friedrichknuth]

I think we can just ignore these globally and add an exception with something like:

**/raw_images/
**/fiducials/
**/preprocessed_images/
**/cropped_images/
**/qc/

!/examples/fiducial_proxy_detection/input_data/fiducials

[godinlu]

Good idea !

[friedrichknuth]

What changed in this notebook? Is there a specific reason to update it?

[friedrichknuth]

What changed in this notebook? Is there a specific reason to update it?

[godinlu]

Not really, I think i juste run it to be sure it work

[friedrichknuth]

OK - thank you for verifying that the notebooks still run as expected. If there are no relevant changes in the notebooks, please just reset the files with git checkout -f path/to/file.ipynb.

[friedrichknuth]

I assume you added this to check for some kind of error? Is there a better place or way to implement the check?

If not, print some kind of error message here that tells the user what is going on. Proper logging gets complicated, but here is an example of how I had been doing it thus far.

[godinlu]

My bad, I forgot to remove it.

[friedrichknuth]

Nice! I see a number of changes here and they look like great improvements, but it is hard to exactly tell. I assume you tested this and it all works well? See additional comments and questions below.

[godinlu]

You right i will simplify it by simply open the img with cv2.imread(image_file_name, cv2.IMREAD_GRAYSCALE). And i will add the linear stretching.

[friedrichknuth]

Is there an advantage to using tifffile? Why did you get rid of the linear stretching?

[friedrichknuth]

The approach to convert RGB to grayscale isn't exactly correct, but I like the conversion to unit8. I assume that improves the performance of the interactive viewer?

[friedrichknuth]

To convert RGB to grayscale, you want to use something like

weights = np.array([0.299, 0.587, 0.114]) 
image_gray = np.tensordot(image_array, weights, axes=([2], [0]))

assuming the image.shape = (height, width, bands) and the band order is Red, Green, Blue. These are the same weights as used by OpenCV

That said, the "color infrared" images have 3 bands that represent NIR, red, and green and I'm not sure what the "best" weights to convert those are. So just taking the same as used for RGB or simply the mean might be fine.

For now, I'd say use the weights above for any 3-band image, and mean for bands > 3 with a note in the doc string.

[friedrichknuth]

What instance does this catch?

[godinlu]

It catch when fiducial_coordinates_true_mm is a numpy array because if you test a numpy array it will raise ValueError: The truth value of an array is ambiguous.

[friedrichknuth]

OK - I think I hadn't run in to this because I provide fiducial_coordinates_true_mm as a list of tuples, not a numpy array in the example.

We could add an explicit check that it is some form of iterable with

from collections.abc import Iterable

if isinstance(fiducial_coordinates_true_mm, Iterable)

[godinlu]

I think is not None is more simple, and it's necessary because fiducial_coordinates_true_mm is converted in np.array here

[friedrichknuth]

OK fine to leave it as is not None. I agree it is simpler!