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

Enable Filtering From List of Profiles #173

Merged
merged 13 commits into from
Jul 10, 2024
Merged

Enable Filtering From List of Profiles #173

merged 13 commits into from
Jul 10, 2024

Conversation

michaelmckinsey1
Copy link
Collaborator

I have came across the need to filter the Thicket based on profiles, and I have been using a roundabout way:

# Filter the metadata table based on list of profiles
tk.metadata = tk.metadata.apply(lambda meta: meta[meta.index.isin(profile_list)], axis=0)
# Synchronize the rest of the components
tk._sync_profile_components(tk.metadata)

So I designed an explicit function for this filtering filter_profile.

@michaelmckinsey1 michaelmckinsey1 added area-thicket Issues and PRs involving Thicket's core Thicket datastructure and associated classes priority-normal Normal priority issues and PRs status-ready-for-review This PR is ready to be reviewed by assigned reviewers type-feature Requests for new features or PRs which implement new features labels Jun 12, 2024
@michaelmckinsey1 michaelmckinsey1 self-assigned this Jun 12, 2024
ilumsden
ilumsden requested changes Jun 12, 2024
View reviewed changes
Copy link
Collaborator

@ilumsden ilumsden left a comment

Choose a reason for hiding this comment

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

Mostly looks good. Just a few minor changes.

thicket/thicket.py Outdated
)

profile_truth = _profile_truth_from_component(component)
self = _sync_indices(component, profile_truth)

return self
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you return self here? Is it for method chaining? If not, I don't really get the purpose of doing this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is no specific reason. I think it could be void. Is there a specific problem you had in mind or just for clarity?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In general, especially given that Python is duck typed, it's better to not return stuff if it's not supposed to be used by the user. So, for example, returning self should only be done if you intend for the user to use that return value for e.g., method chaining. If you start returning stuff when you don't have to, it gets really confusing for users really fast.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets change it to void then.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 1758146

@michaelmckinsey1 michaelmckinsey1 force-pushed the feature-filter_profile branch from 7b2fbe6 to 24af602 Compare June 13, 2024 18:26
@michaelmckinsey1 michaelmckinsey1 mentioned this pull request Jun 13, 2024
Merged
ilumsden
ilumsden approved these changes Jun 17, 2024
View reviewed changes
Copy link
Collaborator

@ilumsden ilumsden left a comment

Choose a reason for hiding this comment

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

I'd still remove the return self (as I mentioned in the last review), but otherwise this looks good

@ilumsden ilumsden added status-approved No more revisions are required on this PR and it is ready for merge and removed status-ready-for-review This PR is ready to be reviewed by assigned reviewers labels Jun 17, 2024
pearce8
pearce8 requested changes Jun 17, 2024
View reviewed changes
Copy link
Collaborator

@pearce8 pearce8 left a comment

Choose a reason for hiding this comment

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

Are there docs/test/examples for this functionality?

@michaelmckinsey1 michaelmckinsey1 mentioned this pull request Jun 18, 2024
Merged
@michaelmckinsey1
Copy link
Collaborator Author

Are there docs/test/examples for this functionality?

No. This PR needs unit tests.

@ilumsden ilumsden added status-ready-for-review This PR is ready to be reviewed by assigned reviewers and removed status-approved No more revisions are required on this PR and it is ready for merge labels Jun 20, 2024
@michaelmckinsey1 michaelmckinsey1 added status-revisions-needed Revisions have been requested from a reviewer for this PR and removed status-ready-for-review This PR is ready to be reviewed by assigned reviewers labels Jun 20, 2024
@michaelmckinsey1 michaelmckinsey1 force-pushed the feature-filter_profile branch 2 times, most recently from d6acfa5 to 1758146 Compare June 21, 2024 03:31
@michaelmckinsey1
Copy link
Collaborator Author

michaelmckinsey1 commented Jul 2, 2024 •
edited
Loading

Are there docs/test/examples for this functionality?

No. This PR needs unit tests.

Unit tests in f0b2f50

docs LLNL/thicket-tutorial#47

@michaelmckinsey1 michaelmckinsey1 requested a review from pearce8 July 2, 2024 21:09
@michaelmckinsey1 michaelmckinsey1 added status-approved No more revisions are required on this PR and it is ready for merge and removed status-revisions-needed Revisions have been requested from a reviewer for this PR labels Jul 3, 2024
Michael Richard Mckinsey and others added 2 commits July 4, 2024 22:33
@michaelmckinsey1 michaelmckinsey1 force-pushed the feature-filter_profile branch from 0d70f3d to 7b776bd Compare July 5, 2024 03:33
@michaelmckinsey1 michaelmckinsey1 mentioned this pull request Jul 9, 2024
Merged
@pearce8 pearce8 merged commit d9a5299 into LLNL:develop Jul 10, 2024
4 checks passed
Yejashi pushed a commit to TauferLab/thicket that referenced this pull request Sep 4, 2024
@michaelmckinsey1 @Yejashi
Enable Filtering From List of Profiles (LLNL#173)
58dbe43 
* Add new function

* Change copy to deepcopy

* Cast type for consistency

* Change _sync_profile_components to void

* Fix bug

* Fix bug

* Add unit tests

* Fix bug

* improve unit test

* black

* Simplify logic

* Fix unit test

* Black

---------

Co-authored-by: Michael Richard Mckinsey <mckinsey@quartz386.llnl.gov>
Co-authored-by: Michael Richard Mckinsey <mckinsey@quartz1922.llnl.gov>
Co-authored-by: Michael Richard Mckinsey <mckinsey@quartz1154.llnl.gov>
Co-authored-by: Michael Richard Mckinsey <mckinsey@quartz2300.llnl.gov>
@slabasan slabasan added this to the 2024.2.0 milestone Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pearce8 pearce8 pearce8 approved these changes

@ilumsden ilumsden ilumsden approved these changes

Assignees

@michaelmckinsey1 michaelmckinsey1

Labels
area-thicket Issues and PRs involving Thicket's core Thicket datastructure and associated classes priority-normal Normal priority issues and PRs status-approved No more revisions are required on this PR and it is ready for merge type-feature Requests for new features or PRs which implement new features
Projects
None yet
Milestone
2024.2.0
Development

Successfully merging this pull request may close these issues.
4 participants
@michaelmckinsey1 @pearce8 @ilumsden @slabasan