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 DataFrame.min and DataFrame.max over 'set_ids' axis or MeshIndex #333

Merged
merged 16 commits into from
Apr 4, 2023

Conversation

PProfizi
Copy link
Contributor

No description provided.

@PProfizi PProfizi added the enhancement New feature or request label Mar 13, 2023
@PProfizi PProfizi self-assigned this Mar 13, 2023
@PProfizi PProfizi changed the title Add DataFrame.min and DataFrame.max over time-Index and MeshIndex Add DataFrame.min and DataFrame.max over "time"-Index and MeshIndex Mar 13, 2023
@PProfizi PProfizi changed the title Add DataFrame.min and DataFrame.max over "time"-Index and MeshIndex Add DataFrame.min and DataFrame.max over 'set_ids' axis or MeshIndex Mar 13, 2023
@PProfizi PProfizi requested review from germa89 and cbellot000 March 13, 2023 17:04
@PProfizi PProfizi requested a review from MaxJPRey March 14, 2023 14:24
df = simulation.displacement(all_sets=True)
# Over the mesh entities
min_over_mesh = [[-0.00074732, -0.00040138, -0.00021555]]
assert np.all(np.isclose(df.min()._fc[0].data.tolist(), min_over_mesh))
Copy link
Contributor

@MaxJPRey MaxJPRey Mar 16, 2023

Choose a reason for hiding this comment

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

Is it expected to expose and use a protected attribute _fc for the field containers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MaxJPRey not to the user. We do have ._core_object property though which we will implement for all classes wrapping a Core entity to give easy access in case it is needed.
Here in the tests this allows me to compare the result of the max operation with doing the same thing directly on the data held by the underlying fields container.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, got it. Thanks for the info.

Comment on lines 846 to 847
Defaults to the MeshIndex (0).
Can also be the SetIndex (1).
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe here we could add additional info just to remind what are the MeshIndex and SetIndex for new users.
I don't remember if it is done somewhere else.

@codecov
Copy link

codecov bot commented Mar 20, 2023

Codecov Report

Merging #333 (5f8c145) into master (49b8eef) will increase coverage by 0.22%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #333      +/-   ##
==========================================
+ Coverage   82.72%   82.94%   +0.22%     
==========================================
  Files          35       35              
  Lines        3173     3214      +41     
==========================================
+ Hits         2625     2666      +41     
  Misses        548      548              

@PProfizi PProfizi added this to the v0.3.1 milestone Mar 23, 2023
@PProfizi PProfizi requested a review from MaxJPRey March 29, 2023 08:03
"""
.. _ref_compute_statistics_example:

Compute statistics of a DataFrame
Copy link
Contributor

Choose a reason for hiding this comment

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

@PProfizi I don't see the mean in this example, can we ranme compute min max then? Adding @JennaPaikowsky to review

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cbellot000 the idea was to create this example now with min and max, and add to it as new features like mean are done

Copy link
Contributor

@cbellot000 cbellot000 Apr 3, 2023

Choose a reason for hiding this comment

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

I understand but I think statistics is bit misleading. Personally, when I'm looking to compute a min max over time (which is a recurring need), I don't search for "statistics"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cbellot000 I understand. I am renaming it, and we will see how to reorganize once there are more features.

@PProfizi PProfizi merged commit ac0375a into master Apr 4, 2023
@PProfizi PProfizi deleted the feat/dataframe_min_max_mean branch April 4, 2023 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants