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

Fixed docs (n2 diagram) #319

Merged
merged 6 commits into from
Jun 15, 2020
Merged

Fixed docs (n2 diagram) #319

merged 6 commits into from
Jun 15, 2020

Conversation

kanekosh
Copy link
Contributor

Purpose

Fixed the documentation to accommodate a new format of n2 diagram in Aerodynamic Optimization Walkthrough.

Type of change

  • Documentation update

Testing

Local build succeeded.

@kanekosh kanekosh requested a review from a team as a code owner June 15, 2020 16:38
ewu63
ewu63 previously approved these changes Jun 15, 2020
Copy link
Collaborator

@ewu63 ewu63 left a comment

Choose a reason for hiding this comment

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

Looks good to me. I don't think there's a way to do a PR build with GitHub Pages unfortunately. Was this the only N2 diagram in the docs?

@kanekosh
Copy link
Contributor Author

Yes this was the only one.

@coveralls
Copy link

coveralls commented Jun 15, 2020

Coverage Status

Coverage remained the same at 95.877% when pulling 247068e on kanekosh:fix_docs into eb9d2b4 on mdolab:master.

@ewu63
Copy link
Collaborator

ewu63 commented Jun 15, 2020

I think it would be good to get @johnjasa to double check this, as I'm not really familiar with how sphinx works with Travis/GitHub Pages for building/deploying docs.

@johnjasa
Copy link
Member

Thanks for this fix, Shugo. I like how you're using the embed-n2 directive from OM now.

I tried building the docs locally in the openaerostruct/docs folder by using make html, but they failed with this message:

  File "/mnt/c/Users/jjasa/git/OpenMDAO/openmdao/docs/_exts/embed_n2.py", line 56, in run
    raise IOError('File does not exist({0})'.format(np))
OSError: File does not exist(/mnt/c/Users/jjasa/git/OpenAeroStruct/openaerostruct/docs/openaerostruct/docs/aero_walkthrough/generate_n2.py)

Basically, the path was wrong to the generate_n2.py file with duplicate openaerostruct/docs parts there. Does the local build work for you? This might be something from before and not from your changes. I'll keep looking at it a bit.

@johnjasa
Copy link
Member

Don't try tackling it - I think I've got it. Can fix it locally in just a bit and I'll push to a branch.

@kanekosh
Copy link
Contributor Author

Thanks John for checking this! The local build actually fails for me as well. When we build locally in openaerostruct/docs, the path origin is openaerostruct/docs. However with Travis, it seems the origin is OpenAeroStruct and Travis test fails in that case.
But I guess I'm missing something as your embed-code works but embed-n2 does not.

@johnjasa
Copy link
Member

I see, that's right on the different origin directories. Do you think the paths I changed in aero_walkthrough.rst will work for Travis, then? They work locally, but it seems there might be different behavior.

Does us including the docs path here help that at all? Maybe I'm misinterpreting what that does, it's been a while.

@johnjasa
Copy link
Member

I guess what it comes down to is that it'd be nice if the docs can be made both on Travis and locally, in my opinion. Do you think that's worth sorting out, or should we say Travis-only is okay?

@kanekosh
Copy link
Contributor Author

I think it will fail, at least for embed-code. Actually my initial commit was similar and it failed.
But since the paths seem to behave differently between embed-code and embed-n2 when built locally, I don't know what will happen with Travis.
I agree, we should be able to check if Travis works by doing make html locally, otherwise the same thing like this will happen and bother reviewers again.

@johnjasa
Copy link
Member

Oof, I should've scrolled up and checked that. Thanks for clarifying. I've pushed a fix to the .travis.yml to change directories, hopefully it works.

@johnjasa
Copy link
Member

Neat, the build looked okay on Travis. All good in my book!

@johnjasa
Copy link
Member

Wait, Coveralls looks wacky still: https://coveralls.io/jobs/63931317
I tried to fix it but I'm not certain if that'll do it. I see it was reported as 0% earlier, too.
@kanekosh, do you have any ideas for that?

@kanekosh
Copy link
Contributor Author

make html works on my local too, thanks!
I think Coveralls will work fine this time. When I first passed Travis build (but not locally) in 94c9498, where Travis was working on OpenAeroStruct dir, there was no problem.

@johnjasa
Copy link
Member

Okay, great! If Coveralls is still weird, I'll leave it up to you to handle. :)

@ewu63
Copy link
Collaborator

ewu63 commented Jun 15, 2020

Is this PR ready to be merged? Happy to do so as long as you guys ironed out the issues @johnjasa @kanekosh

@kanekosh
Copy link
Contributor Author

It's ready!

@ewu63 ewu63 merged commit 1e78deb into mdolab:master Jun 15, 2020
@kanekosh kanekosh deleted the fix_docs branch July 11, 2020 00:32
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.

None yet

4 participants