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

Remove return of scars_mask from remove_scars() #714

Open
ns-rse opened this issue Nov 3, 2023 · 3 comments
Open

Remove return of scars_mask from remove_scars() #714

ns-rse opened this issue Nov 3, 2023 · 3 comments
Assignees
Labels
good first issue Good for newcomers

Comments

@ns-rse
Copy link
Collaborator

ns-rse commented Nov 3, 2023

In reviewing #711 it was identified that there is no use for the returned first_marked_mask as it is not used in any subsequent processing. Commit #1ffe0c therefore assigns it to _ which is discarded.

Rather than return something and discard it we should perhaps consider removing the mask from being returned at all and changing line 434 to...

    return img

As an aside I noticed the typehint for the remove_scars() function doesn't have an explicit return type which if this change is made would be npt.NDArray (I discovered Numpy now has a specific numpy.typing module which if import numpy.typing as npt would allow use of the generic NDArray). The docstring also says self.img is returned when it should, after this change, just be...

Returns
-------
npt.NDArray
    ...

You're thoughts on whether there is ever any use for the scar mask would be appreciated @SylviaWhittle.

@SylviaWhittle
Copy link
Collaborator

So I think we do use the returned scar mask in the second instance, where one of the images for the Filters' images dictionary is set to "scar_mask":
image

But it isn't plotted even with 'all' as the image set.

When developing this feature I had thought it would be of interest to users to see the scars that the scar removal had removed, however given that it was never plotted and that nobody has asked / complained about it, perhaps it should be removed?

I personally have used this a couple times, but not more than that, and as a developer, it's simple to add it back in when needed.

Less code is better

@ns-rse
Copy link
Collaborator Author

ns-rse commented Nov 7, 2023

Less code is better, the balance for me is how much demand there is for the scar masks. ⚖️

@ns-rse
Copy link
Collaborator Author

ns-rse commented Nov 10, 2023

What do you think about keeping these arrays or not? I noticed mention in #675 about processing .asd taking up ~2GB of RAM so keeping these would only inflate that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants