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

Add keyword hide_f_block: bool = None (La and Ac series) to ptable plotters #140

Merged
merged 17 commits into from
May 11, 2024

Conversation

DanielYang59
Copy link
Collaborator

@DanielYang59 DanielYang59 commented May 8, 2024

Summary

TODO

  • Update other ptable plotters (only those with new implementation)
  • Update README to include one example of this functionality

@DanielYang59 DanielYang59 marked this pull request as ready for review May 10, 2024 09:43
@DanielYang59 DanielYang59 requested a review from janosh May 10, 2024 09:43
Copy link
Owner

@janosh janosh left a comment

Choose a reason for hiding this comment

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

this looks great! 👍 all that's missing are some unit tests

pymatviz/ptable.py Outdated Show resolved Hide resolved
@janosh janosh added enhancement New feature or request ptable Periodic table needs testing Needs test coverage labels May 10, 2024
@@ -793,6 +796,9 @@ def tick_fmt(val: float, _pos: int) -> str:
) | (text_style or {})

for symbol, row, column, *_ in df_ptable.itertuples():
if hide_f_block and (row in (6, 7)):
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought I would do this after refactor all other plotters with the new PTableProjector, but thanks for fixing this.

@DanielYang59
Copy link
Collaborator Author

this looks great! 👍

Thanks for reviewing.

all that's missing are some unit tests

Exactly, but I found it tricky to write unit tests for plotters. Do you have any suggestions?

Assert a Figure being generated doesn't really check the results of a plotter. And I have to visually inspect the result (this sounds quite hard to be automated, unlike data processors).

@janosh
Copy link
Owner

janosh commented May 10, 2024

Assert a Figure being generated doesn't really check the results of a plotter. And I have to visually inspect the result (this sounds quite hard to be automated, unlike data processors).

i would test that if hide_f_block is True, the plot contains 30 fewer element tiles and maybe confirm that all the lanthanide and actinide element symbols are absent in labels = {txt.get_text() for txt in ax.texts}

@DanielYang59
Copy link
Collaborator Author

Sounds great. I would add this later :)

@janosh
Copy link
Owner

janosh commented May 10, 2024

Sounds great. I would add this later :)

great. i think once this is merged, we're ready for a v0.8.2 release so people who install from pypi can start using ptable_heatmap_splits.

@DanielYang59 DanielYang59 requested a review from janosh May 11, 2024 09:57
Copy link
Owner

@janosh janosh left a comment

Choose a reason for hiding this comment

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

i think we can be a bit smarter about the hide_f_block default behavior. let's set it to None by default and then use True or False depending on whether we have non-zero values for any rare earths. passing True or False explicitly of course will override this behavior

pymatviz/ptable.py Show resolved Hide resolved
Copy link
Owner

Choose a reason for hiding this comment

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

nice, much better now. didn't actually notice all the white space before

@DanielYang59
Copy link
Collaborator Author

i think we can be a bit smarter about the hide_f_block default behavior. let's set it to None by default and then use True or False depending on whether we have non-zero values for any rare earths. passing True or False explicitly of course will override this behavior

Excellent idea. Let's do it.

for atom_num in [*range(57, 72), *range(89, 104)] # rare earths
# check if data is present for f-block elements
if (elem := Element.from_Z(atom_num).symbol) in self.data.index # type: ignore[union-attr]
and self.data.loc[elem, "Value"] # type: ignore[union-attr]
Copy link
Owner

@janosh janosh May 11, 2024

Choose a reason for hiding this comment

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

one more thing to change here: it's always a good to maintain SSOT to avoid drift and to ensure it remains easy to rename things in the future. the "Value" key is hard-coded in many places in this file. would be better to add a new key in init.py

Copy link
Collaborator Author

@DanielYang59 DanielYang59 May 11, 2024

Choose a reason for hiding this comment

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

Yes, as we're hard coding the column names:

pymatviz/pymatviz/ptable.py

Lines 217 to 254 in 39fd71e

def data_preprocessor(data: SupportedDataType) -> pd.DataFrame:
"""Preprocess input data for ptable plotters, including:
- Convert all data types to pd.DataFrame.
- Impute missing values.
- Handle anomalies such as NaN, infinity.
- Write vmin/vmax as metadata into the DataFrame.
Returns:
pd.DataFrame: The preprocessed DataFrame with element names
as index and values as columns.
Example:
>>> data_dict: dict = {
"H": 1.0,
"He": [2.0, 4.0],
"Li": [[6.0, 8.0], [10.0, 12.0]],
}
OR
>>> data_df: pd.DataFrame = pd.DataFrame(
data_dict.items(),
columns=["Element", "Value"]
).set_index("Element")
OR
>>> data_series: pd.Series = pd.Series(data_dict)
>>> preprocess_data(data_dict/df/series)
Element Value
0 H [1.0, ]
1 He [2.0, 4.0]
2 Li [[6.0, 8.0], [10.0, 12.0]]
Metadata:
vmin: 1.0
vmax: 12.0
"""

@janosh janosh changed the title Allow hiding f-block (La and Ac series) in ptable plotter Add keyword hide_f_block: bool = None (La and Ac series) to ptable plotters May 11, 2024
Copy link
Owner

@janosh janosh left a comment

Choose a reason for hiding this comment

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

thanks so much for this @DanielYang59! 👍

@janosh janosh removed the needs testing Needs test coverage label May 11, 2024
@janosh janosh merged commit eb61ecf into janosh:main May 11, 2024
8 checks passed
@DanielYang59 DanielYang59 deleted the ptable_hide_la-ac branch May 11, 2024 15:25
@@ -235,8 +239,8 @@ def data_preprocessor(data: SupportedDataType) -> pd.DataFrame:
OR
>>> data_df: pd.DataFrame = pd.DataFrame(
data_dict.items(),
columns=["Element", "Value"]
).set_index("Element")
columns=[{element_col}, {value_col}]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It looks like this breaks the site builder workflow. I should have used a raw string instead. Please help me fix this. Thanks!

Copy link
Owner

Choose a reason for hiding this comment

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

will fix, PR incoming for SSOT of keys across the whole code base

@DanielYang59
Copy link
Collaborator Author

Great to collaborate with you on this one. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ptable Periodic table
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Several possible improvements for ptable_heatmap plotter
2 participants