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

Improved search.py #9

Merged
merged 9 commits into from
Jun 5, 2023
Merged

Improved search.py #9

merged 9 commits into from
Jun 5, 2023

Conversation

louwersj
Copy link
Contributor

made several improvements to the search.py codebase

@hulsed
Copy link
Collaborator

hulsed commented May 27, 2023

See comment. Overall, search.py is not in a working state anymore/yet with this alpha version so we may have some changes in the pipeline in the next few weeks as we bring the examples up to spec.

@louwersj
Copy link
Contributor Author

@hulsed could you still merge this imporved / cleaned one in the project main

We could properly forked it from that point onwards.

Copy link
Collaborator

@hulsed hulsed left a comment

Choose a reason for hiding this comment

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

Need to adjust some of the changes to FxnBlock

fmdtools/sim/search.py Show resolved Hide resolved
@@ -1048,37 +1046,14 @@ class FxnBlock(Block):
Superclass class for functions which is a special type of Block\
with c and a attributes for CompArch and ASGs, as well as a defined method for propagation
"""
def __init__(self, name='', flows={}, c=dict(), a=dict(), local=dict(), args_f=dict(), **kwargs):
def __init__(self, name='', flows={}, args_f=dict(), **kwargs):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is removing too many attributes and will break certain models/workflows that rely on the ASG and CompArch types:

  • c and a are instantiated just a bit below, so they need to be kept here
  • local needs to be kept (and I think is supposed to call add_local_to_flowdict, which I think is supposed to happen in comms also--so, would make a good issue/bug report)
  • The previous docs for parameters is mostly accurate--p,s,r,m,t, and sp are all parameters that get passed as kwargs to Block. Maybe we need a better way of documenting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving this back into the code

Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like your latest commit only has the changes to search.py--make sure this makes it in and I'll merge the PR

@louwersj
Copy link
Contributor Author

louwersj commented Jun 2, 2023

@hulsed changes are made to correct the requested. Can you have a quick check on the two changes done to search.py and block.py ?

@hulsed
Copy link
Collaborator

hulsed commented Jun 2, 2023

@hulsed changes are made to correct the requested. Can you have a quick check on the two changes done to search.py and block.py ?

I don't see that the corrections to block.py made it in but will once they are there

@louwersj
Copy link
Contributor Author

louwersj commented Jun 5, 2023

adding corrections to block.py @hulsed prime change in 66b5bb0

@hulsed hulsed merged commit d646fca into nasa:main Jun 5, 2023
hulsed added a commit that referenced this pull request Jul 20, 2023
Merge in RAD/fmdtools from feature/RAD-168-create-dsa-analyses-indicator-analyses to 2.0-dev

* commit 'b12744e44432c72def7f7655599e7e9e9b6de485':
  Fixing style issues with graph
  Some formatting fixes to graph
  Some formatting changes per PR comments
  define.mode bugfixes
  Update graph.py
  Update graph.py
  Added indicators to ModelGraphs, etc
  formatting update
  Adding ability to show indicators in results graphs
  Some style changes in flow
  More recordclass Adaptation
  Some bugfixes
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