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.

Sign up for GitHub

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

Jump to bottom

Add featurizer #102

Merged
merged 52 commits into from
Sep 13, 2023
Merged

Add featurizer #102

merged 52 commits into from
Sep 13, 2023

Conversation

naik-aakash
Copy link
Collaborator

@naik-aakash naik-aakash commented Apr 25, 2023 •
edited
Loading

Enhancement > Featurizer for ML #96

Added featurizers for ML

Todo

  • Add FeaturizeLobsterpy class for lightweight jsons (of manuscript)
  • Add FeaturizeCOXX class (fingerprints, w_icohp, moment features)
  • Refactor FeaturizeCOXX class methods
  • Add FeaturizeCharges class (Ionicity)
  • Add tests for FeaturizeLobsterpy
  • Update test of FeaturizeLobsterpy to also check exception case
  • Add tests for FeaturizeCOXX
  • Add tests for FeaturizeCharges

@naik-aakash naik-aakash added the enhancement New feature or request label Apr 25, 2023
naik-aakash and others added 27 commits April 25, 2023 16:09
@naik-aakash
update test
a447a86
@naik-aakash
update featurizer to incorporate OOP
1d211a0
@naik-aakash
add test for Lobsterpyjson featurizer
8841394
@naik-aakash
add optional featurizer dependencies
7e906ba
@naik-aakash
update test and publish workflows
5123d02
@naik-aakash
update workflows
c3fe2c6
@naik-aakash
fix linting errors
14d9d27
@naik-aakash
update core.py
fc2ace0
@naik-aakash
mypy error fix
c63a299
@naik-aakash
more mypy error fixes
9ad2a29
@naik-aakash
fix mypy variable assignment error
12204da
@naik-aakash
attempt error ignore temporary
ded6dd2
@naik-aakash
fix mypy,pydocstyle errors for featurizer module
b3e675b
@naik-aakash
update __init__.py
2a2b8e4
@naik-aakash
refactor featurizer module, added summary.py and batch.py module, add…
f0e0d14 
…ed coxx featurizer test
@naik-aakash
fix docstring
7dea944
@naik-aakash
fix liniting errors
5d489f3
@naik-aakash
refactor featurize module, add more tests
2f61e81
@naik-aakash
fix pylint import errors
ccbf17e
@naik-aakash
run black
c5245f3
@naik-aakash
Merge branch 'JaGeo:main' into add-featurizer
6030c33
@naik-aakash
sort index of dataframe returned by batch featurizers
b568190
@naik-aakash
run black
19937e6
@naik-aakash
Merge branch 'add-featurizer' of github.com:naik-aakash/LobsterPy int…
ce21ab6 
…o add-featurizer

sync remote
@naik-aakash
added .coveragerc, as multiprocessing calls are not captured without it
a7d0889
@naik-aakash
update workflow to get correct coverage report
3499229
@naik-aakash
add missing doc string
40656f1
@naik-aakash
Copy link
Collaborator Author

naik-aakash commented May 26, 2023 •
edited
Loading 

coverage run -m pytest
coverage combine
coverage report

Need to use this 3 commands successively to get correct value of the coverage (Low coverage we are seeing is coz of multiprocessing module)

@JaGeo
Copy link
Owner

JaGeo commented May 26, 2023 •
edited
Loading

I will check again. Thanks. However, we could still to increase the coverage further. By refactoring, you might be able to get even more. There are some code pieces that are called multiple times.

JaGeo
JaGeo reviewed May 26, 2023
View reviewed changes
lobsterpy/featurize/core.py

def _get_lobsterpy_cba_dict(self) -> dict:
"""
This function uses lobsterpy.cohp.analyze.Analysis class to generate a python dictionary object
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

method?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Changed it now to a static method.

JaGeo
JaGeo reviewed May 26, 2023
View reviewed changes
lobsterpy/featurize/core.py Outdated
COXX nth moment in eV
"""
if e_range:
coxx = coxx[(energies >= self.e_range[0]) & (energies <= self.e_range[-1])]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It probably works but I would still make sure this comparison is okay.

JaGeo
JaGeo reviewed May 26, 2023
View reviewed changes
lobsterpy/featurize/core.py Outdated
if ids:
df = pd.DataFrame(index=[ids])
else:
ids = os.path.basename(os.path.dirname(self.path_to_coxxcar))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In many parts of the code, we now use "Path". Could you update this?

Copy link
Collaborator Author

@naik-aakash naik-aakash Jun 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made it consistent now

@JaGeo
Copy link
Owner

JaGeo commented May 26, 2023

Could Ou also add documentation of the new installation process?

@naik-aakash
Copy link
Collaborator Author

Could Ou also add documentation of the new installation process?

I will do it in another PR and link it to this open issue #117

@naik-aakash naik-aakash changed the title [WIP] Add featurizer Add featurizer Jun 5, 2023
@naik-aakash
Copy link
Collaborator Author

Hi @JaGeo , I think now the issues with numerical stability should be fixed and also more tests have been added to improve the coverage. If any more changes necessary would be happy to address it 😃

@naik-aakash
Copy link
Collaborator Author

Also, I will try to add orbital-wise COXX features (center, width, skew, kurtosis and fingerprints) as well . But will be added in another PR once this is merged.

@naik-aakash naik-aakash mentioned this pull request Jul 31, 2023
Merged
7 tasks
JaGeo
JaGeo reviewed Sep 6, 2023
View reviewed changes
pylintrc
@@ -78,67 +72,6 @@ disable=print-statement,
useless-suppression,
deprecated-pragma,
use-symbolic-message-instead,
apply-builtin,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@naik-aakash any reason why we have to remove all of them or is this an issue with not having the latest main branch merged?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I removed them as they seem deprecated and made the lint workflow print a whole lot of warnings,

JaGeo
JaGeo approved these changes Sep 13, 2023
View reviewed changes
Copy link
Owner

@JaGeo JaGeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked throught the main implementations. For the featurization of the COHPs and further features, we might want to run some more tests later on.

@JaGeo JaGeo merged commit e4e317d into JaGeo:main Sep 13, 2023
21 checks passed
@naik-aakash naik-aakash deleted the add-featurizer branch June 20, 2024 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JaGeo JaGeo JaGeo approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Featurizer for ML
2 participants
@naik-aakash @JaGeo