[Breaking] Refactor ptable heatmap plotter #157
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
for more information, see https://pre-commit.ci
DanielYang59
added
ptable
for more information, see https://pre-commit.ci
DanielYang59
changed the title
|
planning a v0.9.0 release in the coming days (minor version bump due to the breaking changes in #161). anything i can help with here to get this merged before then? |
This comment was marked as outdated.
This comment was marked as outdated.
no need to apologize, take your time! 👍 tell your family i said hi 😄 |
DanielYang59
commented
Jun 27, 2024
DanielYang59
commented
Jun 27, 2024
DanielYang59
commented
Jun 27, 2024
DanielYang59
commented
Jul 12, 2024
DanielYang59
commented
Jul 12, 2024
| @@ -26,6 +26,8 @@ | |||
|
|
|||
| from pymatgen.core import Composition | |||
|
|
|||
| df_ptable = df_ptable.copy() # avoid changing the df in place | |||
There was a problem hiding this comment.
Don't know if there will be a better solution (pandas doesn't seem to provide any method to make a dataframe immutable)?
Also this is pretty interesting (my first time to notice one unit test alters the behavior of another), in this case test_ptable_matplotlib.py seems to alter df_table and therefore test_process_data.py failed (just guessing).
janosh
approved these changes
Jul 21, 2024
There was a problem hiding this comment.
thanks a lot @DanielYang59! 👍 i appreciate all the work that went into this PR!
apologies it took this long too review. there's something to be said for small PRs since it's hard to find a sufficiently long stretch of free time to review large PRs in one go other than weekends. 😄
|
No need to apologize in any case, and thanks for reviewing and the final top up! I really appreciate your time from your weekend to review this. I'm just hoping not too many things would be broken by this PR :) Meanwhile wondering why we're falling back from |
2 tasks
in this case i don't mind for the tests to be sensitive. if a refactor changes exact equality, i think it's good for the tests to alert us to this even if we go with it and then need to update the tests |
|
Sure thing! |
janosh
added a commit
that referenced
this pull request
Mar 28, 2025
* comment out heatmap plotter for refactor * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * group args for refactor * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * remove None as default * remove commented out legacy code * fix unsupported default None value for elem_colors * add missing start_angle arg * add heatmap child plotter * remove Callable type from plot_kwargs * set hide_f_block default to "AUTO" * docstring tweak * add values display functionality * add docstring for heatmap plotter * add fences for blocks in docstring * add log scale preprocessor * warning upon illegal log value * tweak docstring * skip ptable heatmap tests * add more comments * add data sum normalizer * merge heat_mode and show_values to show_values_mode * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * change default symbol font to white and bold * make default cbar title font 16 and bold * fix missing % sign in percent mode * rename show_values_mode to values_show_mode * add tile_colors to store and overwrite tile colors * add values f-string formatter * add setting cbar range, need double check overwrite norm * temp save: add tile_size docstring * change border color to grey * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix value in percent mode * remove percentage mode from normalizer * fix merge main * remove TODO tag * enable cbar label format in percent mode * WIP: separate label fmt generate as method * WIP: add sci_notation arg * add cbar label sci notation formatter * fix sci notation in fraction mode * add TODO tag for tests to migrate * move get_cbar_label_formatter to utils * turn off useMathText for consistency with values in tile * remove percentage arg * add tests for get_cbar_label_formatter * track examples, would remove before merge * overwrite add_child_plots method * access colors from tile_colors property * tiny docstring correction * remove accidentally committed test file * simplify sci notation of e+0 * SUBOPTIMAL: pass tile_size to subplots figsize * WIP, not working: add f-block vertical offset * fix wrong arg passing * separate HMapPTableProjector * preprocess_ptable_data and downstream funcs now record anomaly * remove TODO tag * overload preprocess_ptable_data type * add overwrite_colors generator and unit test * fix log_scale apply, and return type * docstring tweaks * In process: migration some of the heatmap tests * remove TODO tag * fix mypy errors * migrate most unit tests * migrate most unit tests * add overwrite_colors arg * add PTableData and unit test, not fully finished * add drop_elements method * add unit test for apply method * move PTableData to ptable and relocate unit tests * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * add placeholder wrap for from_formulas method * fix nan replacement when nan and inf coexist * fix log scale apply * fix type errors * simplify missing_strategy arg name to strategy * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * mark tile_size and f-block offset as WIP * add text_colors to unify value/symbol colors and fix inconsistency * fix typo in test and pick_bw_for_contrast allow color as str * NEED CONFIRM: remove test for color consistency * rename text_color to text_colors for consistency * migrate asset maker for ptable-heatmap-atomic-mass * migrate ptable-heatmap-percent * fix colormap in percent mode * fix log scale * uncommit example figures * add back exclude_elem * minor clean up * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * add tests to exclude_elems * add overwrite_anomalies to handle nan/inf and tests * remove finished TODO * make exclude_elements in cline * fix tile color and value when on_emtpy is show * increate title font size * remove cbar range to be consistent with previous setting * WIP: migrate ptable_heatmap_ratio * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix arg in other examples * make overwrite_anomalies in line * fix color mapping in log mode * update ratios plot * optimize norm setter * remove excluded elem to avoid metadata pollution and fix text color * fix color mapping in percent/fraction mode * break into submodules * remove debug script * Revert "break into submodules" This reverts commit a43912e. * fix merge main * split ptable projector * add ratio plot legends * fix NaN display in ratio plotter * migrate unit test for ratio plotter * condense tile value, color and text_color generation * WIP: merge tile symbol, color and value plot into add_heatmap_tiles * first working version after tile refactor * fix colorbar format * rename args values_xx to value_xx * add missing docstrings * regenerate asset with new implementation * finish ratio plotter migration * fix wrong import of ptableprojector in unit test * remove HMapPTableProjector into _projectors module * add unit tests for generate_tile_value_colors * fix mypy errors * set NaN default color as lightgrey * add deprecated warning for args * remove inf_color to infty_color for backwards compatibility * add more deprecated args * WIP: migrate matbench_dielectric_eda.ipynb * WIP: migrate mprester_ptable.ipynb * set default on_empty to show and reset normalizer for values not present * fix matbench_dielectric_eda ipynb * ptable_heatmap now filter near zero values internally with a warning * fix value type * migrate mprester_ptable.ipynb * migrate example boltztrap_mp * migrate camd_2022 * set default value_fmt to .0f in log mode to be large-number friendly * fix and migrate ricci_carrier_transport * add debug tag * migrate dielectric * migrate expt_gap * migrate other matbench * simplify count_elements import * fix unit test * remove TODO tag * drop not available structure * add return_type for smoother migration to return figure * reapply value filter for percent mode * revert accidental revert during conflict merge * fix return type in test and add test for return_type * add debug tag * make copy of df * include ax example in asset * remove ptabledata from_formula method * change ptable_matplotlib.py doc str section format # -> --- * rename Literal "AUTO"->"auto" for automatic text color and text format mode * df_ptable.atomic_mass->df_ptable[Key.atomic_mass] * rename HMapPTableProjector->HeatMapPTableProjector * simplify assert isclose(a, b) -> assert a == b * decrease filter_near_zero(tol=1e-4->1e-6) * add isinstance(self.anomalies, dict) to remove need for typing.cast --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Janosh Riebesell <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
ptable_histsandptable_heatmapwith newPTableProjector#142.PTableDataclass to handle data preprocessing in a more systematic way for ptable plotterson_emptyvalue is notshowinstead ofhide, rationale: user might expect a "complete" looking ptable even when data for element is missingUpdate example ipynb
Things for a follow up PR
check_and_replace_missing,check_and_replace_infinityandlog_scale