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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃敤 Remove input_size argument from models #1827

Closed
samet-akcay opened this issue Mar 7, 2024 · 8 comments 路 Fixed by #1856
Closed

馃敤 Remove input_size argument from models #1827

samet-akcay opened this issue Mar 7, 2024 · 8 comments 路 Fixed by #1856
Assignees
Labels
Good First Issue Issues that can be picked up by someone unfamiliar with the repo and would like to contribute. Refactor Refactoring is needed
Milestone

Comments

@samet-akcay
Copy link
Contributor

Description

After merging PR #1706, models no longer require input_size argument. However, there are some places where input_size is still used, such as docs and some other utility functions. This is to ensure they are all removed properly.

For example, here is an example to the Patchcore model.

class Patchcore(MemoryBankMixin, AnomalyModule):
"""PatchcoreLightning Module to train PatchCore algorithm.
Args:
input_size (tuple[int, int]): Size of the model input.
Defaults to ``(224, 224)``.
backbone (str): Backbone CNN network
Defaults to ``wide_resnet50_2``.
layers (list[str]): Layers to extract features from the backbone CNN

@samet-akcay samet-akcay added Refactor Refactoring is needed Good First Issue Issues that can be picked up by someone unfamiliar with the repo and would like to contribute. labels Mar 7, 2024
@samet-akcay samet-akcay added this to the v1.1.0 milestone Mar 7, 2024
@Shakib-IO
Copy link
Contributor

Hi @samet-akcay
I want to work on this issue.

@samet-akcay
Copy link
Contributor Author

Sure, thanks for your interest

@Shakib-IO
Copy link
Contributor

Shakib-IO commented Mar 10, 2024

Hi @samet-akcay,
I've begun addressing the issue at hand and have a few inquiries.

Certain utility functions contain the parameter input_size, as seen here (cfa). Should I also eliminate these instances?

Additionally, some functions compute values dependent on input_size, like here here (ganomaly) Should these calculations be adjusted as well?

Thanks

@dyogaharshitha
Copy link

.take

@Shakib-IO
Copy link
Contributor

Shakib-IO commented Mar 11, 2024

Hi @samet-akcay
In ganomaly, the encoder and decoder have the input_size, based on the input_size it calculates some values. Should these calculations be removed and adjusted as well? Please let me know.

Thanks.

@Shakib-IO
Copy link
Contributor

Hi @blaz-r
Any comment on this?

@blaz-r
Copy link
Contributor

blaz-r commented Mar 14, 2024

Hi,
about the cfa question you had I think that this input_size stays as it's internal.

About ganomaly, the input_size you linked is part of torch model and that needs to stay. You should remove the input_size in lighting_model docstring:

class Ganomaly(AnomalyModule):
"""PL Lightning Module for the GANomaly Algorithm.
Args:
input_size (tuple[int, int]): Input dimension.
Defaults to ``(256, 256)``.

the input_size is set inside lighting_model with setup function:
def _setup(self) -> None:
assert self.input_size is not None, "CSflow needs input size to build torch model."
self.model = GanomalyModel(
input_size=self.input_size,
num_input_channels=3,
n_features=self.n_features,
latent_vec_size=self.latent_vec_size,
extra_layers=self.extra_layers,
add_final_conv_layer=self.add_final_conv_layer,

so the code you linked will work. But looking at this setup function, it seems like the assert string is wrong as it says csflow, so you can fix that.

@Shakib-IO
Copy link
Contributor

Hi @blaz-r
Thanks.

Please review the PR and let me know.
Thank you.

Shakib-IO added a commit to Shakib-IO/anomalib that referenced this issue Mar 17, 2024
Signed-off-by: Shakib-IO <shakib.khan17@northsouth.edu>
samet-akcay added a commit that referenced this issue Mar 18, 2024
Signed-off-by: Shakib-IO <shakib.khan17@northsouth.edu>
Co-authored-by: Samet Akcay <samet.akcay@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue Issues that can be picked up by someone unfamiliar with the repo and would like to contribute. Refactor Refactoring is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants