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

Update configs to use dataclasses #2093

Open
3 of 15 tasks
johnnv1 opened this issue Dec 16, 2022 · 11 comments · Fixed by #2899
Open
3 of 15 tasks

Update configs to use dataclasses #2093

johnnv1 opened this issue Dec 16, 2022 · 11 comments · Fixed by #2899
Labels
code heatlh 💊 Improvement the package code health enhancement 🚀 Improvement over an existing functionality good first issue Good for newcomers help wanted Extra attention is needed

Comments

@johnnv1
Copy link
Member

johnnv1 commented Dec 16, 2022

We have some specific configs for some algorithms, will be nice to update them from using dicts/TypedDict to dataclasses.

The idea here is to do it in a way that does not break things, so we should have an interface (to/from) between dict and the dataclasses.

Example of what we can explore for these methods

>>> from dataclasses import dataclass, asdict
>>> @dataclass
... class A:
...     b: int
... 
>>> asdict(A(1))
{'b': 1}
>>> A(**asdict(A(1)))
A(b=1)

Originally posted by @johnnv1 in #2092 (comment)

List of some configs to be replaced:

@johnnv1 johnnv1 added enhancement 🚀 Improvement over an existing functionality help wanted Extra attention is needed good first issue Good for newcomers code heatlh 💊 Improvement the package code health labels Dec 16, 2022
@edgarriba
Copy link
Member

@ducha-aiki can you help to triage importance in the feature module. Also to audit if we eventually have things to deprecate etc

@Ikonsty
Copy link

Ikonsty commented Mar 2, 2023

@ johnnv1 Can I fix one of the files, for example, KeyNetConf, pull it, and if everything is okay, add more changes?

@Ikonsty
Copy link

Ikonsty commented Mar 2, 2023

/assign Ikonsty

@johnnv1
Copy link
Member Author

johnnv1 commented Mar 2, 2023

Go ahead, when the PR is ok for review you can ping me and @ducha-aiki for it 😄

@rajshukla1102
Copy link

hey @johnnv1 can you assign this to me

@johnnv1
Copy link
Member Author

johnnv1 commented Mar 27, 2023

@rajshukla1102 Go ahead, but if possible, open separate PRs for each feature

@rajshukla1102
Copy link

@johnnv1 Thank you! looking forward to completing this soon😊

@lappemic
Copy link
Contributor

lappemic commented Apr 9, 2024

I've been reviewing the list of potential contributions and noticed that many tasks are either already completed or no longer relevant. However, I've identified a couple of areas where I believe I could contribute effectively:

  • kornia.feature.matching._get_default_fginn_params
  • kornia.feature.adalam.core.AdalamConfig

Could you please confirm if these contributions are still needed? I would appreciate any additional guidance or suggestions you might have on how to proceed.

Looking forward to your feedback!

@johnnv1
Copy link
Member Author

johnnv1 commented Apr 9, 2024

Could you please confirm if these contributions are still needed?

Sure it's, place a PR for this, and if you have any questions you can ping me at kornia slack

For cases like AdalamConfig which is a typed dict within NoRequired, you will need to check if we will need to initialize it somehow or not. Based on this, you'll be able to define the types, and if necessary you'll need to add some isinstance checks in some places

aside from the typing, the asdict <-> config conversion as in the issue, should work fine

@lappemic
Copy link
Contributor

lappemic commented Apr 15, 2024

In #2880 the kornia.feature.sold2.sold2_detector.default_cfg config is refactored. I therefore also looked into kornia.feature.sold2.sold2.default_cfg:

  • kornia.feature.sold2.sold2.default_cfg uses the same default_config as in SOLD2_detector plus the line_matcher_cfg. Shall we put the dataclass definitions in a separate file (e.g. sold2_dataclasses.py in utils)? This way we could avoid repetition.

@johnnv1
Copy link
Member Author

johnnv1 commented Apr 16, 2024

Shall we put the dataclass definitions in a separate file (e.g. sold2_dataclasses.py in utils)? This way we could avoid repetition.

Yeap, the detector default config has the descriptor equals False different than other one. But you can unify it under a structures.py file in sold2 module ("kornia/feature/sold2/structures.py").

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code heatlh 💊 Improvement the package code health enhancement 🚀 Improvement over an existing functionality good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants