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

Better error checking for connect function #28

Merged
merged 6 commits into from
Apr 1, 2020

Conversation

JustinSGray
Copy link
Contributor

Purpose

I noticed users passing incorrect signatures to the connect call, but no errors were being thrown. The incorrect signature had strings being assigned to the label_width argument, and then not showing up in the XDSM.

I added an error check so label_width must be an integer (or None) now.

I also made the json spec generator ignore empty strings.

Type of change

  • Bugfix (non-breaking change which fixes an issue)

Testing

Checklist

Put an x in the boxes that apply.

  • [x ] I have run unit and regression tests which pass locally with my changes
  • I have added new tests that prove my fix is effective or that my feature works
  • I have added necessary documentation

Note: documentation not required for this bug fix.

@JustinSGray JustinSGray requested a review from a team as a code owner April 1, 2020 18:42
@ghost ghost requested a review from marcomangano April 1, 2020 18:42
@ewu63
Copy link
Collaborator

ewu63 commented Apr 1, 2020

PR looks good. A few (unrelated) comments:

  • we should probably just remove those PDF right? Doesn't make sense to keep them in the repo
  • can you explain a bit what the json spec file does? I didn't really understand it at the time you merged it in

ewu63
ewu63 previously approved these changes Apr 1, 2020
@ehariton
Copy link

ehariton commented Apr 1, 2020

The .json spec files created from the XDSM allow for test verification that the individual subsystems shown in the XDSM are meeting the input and outputs defined by the XDSM. This is achieved with a call to assert_match_spec() which is a util function we made. This is useful from the standpoint of you can have a team sit around and update and agree to an XDSM configuration. And then as that team builds subsystems, they are required to meet those XDSM input/output variables exactly or their tests will fail and their PRs won't be merged in. Hope that helps a bit!

@ewu63
Copy link
Collaborator

ewu63 commented Apr 1, 2020

@ehariton thanks for the description, that makes sense. Is the function assert_match_spec() something internal to the openMDAO team, or is it publicly available somewhere?

At a minimum I think we should rephrase what you wrote and put it into the docstring. In the long term I will look into setting up a documentation site, since this repo is used by many but has virtually no documentation anywhere.

@ehariton
Copy link

ehariton commented Apr 1, 2020

I'm sure Justin could phrase it more eloquently. Right now we have assert_match_spec() in a private repo.

@JustinSGray
Copy link
Contributor Author

JustinSGray commented Apr 1, 2020

we're prototyping the assert_match_spec function in another repo. Its still under active development, but id be happy to add it to the pyXDSM repo. Its coded to work specifically with OpenMDAO, so maybe it would be better suited to be in the OM repo.

The spec is super general though. I'll add one to this PR

Normally I don't like having the PDFs in the repo, but for the examples in this repo I think its valuable because it allows people to download them without having latex... so it makes it easier for them to evaluate if they want to try out the codebase or not. If we had some real docs and added images then the PDFs could go away... IMO

@ewu63
Copy link
Collaborator

ewu63 commented Apr 1, 2020

Thanks for the write up Justin. The point about the PDFs also make sense -- let's keep them in until we have docs up. Then we can build them when we build docs and add them to the site.

@ewu63 ewu63 merged commit fab6e3a into mdolab:master Apr 1, 2020
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.

3 participants