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

exprs refactor and reorg part 1 #1167

Merged
merged 14 commits into from
Jul 14, 2023
Merged

exprs refactor and reorg part 1 #1167

merged 14 commits into from
Jul 14, 2023

Conversation

cyrush
Copy link
Member

@cyrush cyrush commented Jul 7, 2023

No description provided.

@cyrush cyrush marked this pull request as draft July 7, 2023 19:33
@cyrush
Copy link
Member Author

cyrush commented Jul 7, 2023

Provides renames for both class names and filter graph names for most expression filters.
Reorg of implementation order with better logical grouping of functionality.

@cyrush cyrush marked this pull request as ready for review July 11, 2023 22:16
@cyrush
Copy link
Member Author

cyrush commented Jul 11, 2023

@nicolemarsaglia FYI: Reorg with name changes, etc.

Post this PR I will make more extensive additions.

topo_sig["return_type"] = "topo";
topo_sig["filter_name"] = "topo";
topo_sig["return_type"] = "topology";
topo_sig["filter_name"] = "expr_mesh_topology";
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this have the same filter name as topolgoy_sig below it (line 977)?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, think of these as two aliases that map to the same filter.

However, I typo below -- we need to set params for:

topology_sig instead of topo_sig, or else topology_sig isn't inited.

The original function name was only topo, but I wanted us to be able to call the more explicit topology as well.

The return type for both has to be topology, b/c that is the new name I deemed moving forward for the expression language object that holds the topology.

Copy link
Contributor

Choose a reason for hiding this comment

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

Saw your "?" comments on two of the args. Not sure if you wanted to change those now or wait
1389: field to scalar
1273: scalar to field

Copy link
Member Author

@cyrush cyrush Jul 12, 2023

Choose a reason for hiding this comment

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

Yes - thanks, I am still working on those two.

Copy link
Contributor

@nicolemarsaglia nicolemarsaglia left a comment

Choose a reason for hiding this comment

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

the renaming is so so so so helpful

@cyrush
Copy link
Member Author

cyrush commented Jul 14, 2023

Getting this chunk in to open up the floor to more changes.

@cyrush cyrush merged commit 3223678 into develop Jul 14, 2023
19 checks passed
@cyrush cyrush deleted the task/2023_07_expr_org branch July 14, 2023 21:10
@cyrush cyrush changed the title WIP: exprs refactor and reorg exprs refactor and reorg part 1 Jul 14, 2023
@cyrush cyrush mentioned this pull request Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants