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

Minor changes #133

Merged
merged 3 commits into from
Jun 4, 2019
Merged

Minor changes #133

merged 3 commits into from
Jun 4, 2019

Conversation

xiaoruiDong
Copy link
Contributor

I was trying to install the ARC based on the instruction from Wiki and encountered errors when running ESS diagnostics:

  • Change the isomorphic_species_list to same_species_list to keep up with the latest update of RMG
  • Avoid error when running into local address error (double checking, but treat local as SSH) in ESS check
  • No ARC.determine_remote exists. I guess that it updates to ARC.determine_ess_settings, so I changed it. I also removed the output of iPython notebook, but it can be kept if you want.

@codecov
Copy link

codecov bot commented Jun 3, 2019

Codecov Report

Merging #133 into master will decrease coverage by 0.01%.
The diff coverage is 16.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #133      +/-   ##
==========================================
- Coverage   41.03%   41.01%   -0.02%     
==========================================
  Files          23       23              
  Lines        5888     5890       +2     
  Branches     1567     1568       +1     
==========================================
  Hits         2416     2416              
- Misses       3082     3084       +2     
  Partials      390      390
Impacted Files Coverage Δ
arc/main.py 45.19% <0%> (-0.13%) ⬇️
arc/rmgdb.py 73.5% <25%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8e6dc2...55ad23a. Read the comment docs.

@alongd
Copy link
Member

alongd commented Jun 4, 2019

Thanks for the contribution!

I think the iPython output should stay, as an example of what to expect from this tool, unless it doesn't show well. If we do decide to remove it, then it should have its own commit.

I understand that the ESS check is buggy when running on a server. How does the addition of

            if server == 'local':
                continue

treat local as SSH? I think the expected behaviour is that we will be able to determine ESS when running locally as well.

@xiaoruiDong
Copy link
Contributor Author

@alongd Thanks for quick reply!

  1. I agree it can stay. So, I update the ipython notebook with result being displayed (based on my settings)
  2. About ESS. Actually, it has two steps regarding servers:
    First, it checks whether 'local' is in the server list, and, if so, it will search which packages are available locally.
    Screenshot from 2019-06-04 10-30-04

Second, it will loop through the server list and tell what packages are available remotely.
Screenshot from 2019-06-04 10-53-35

However, within the loop, 'local' is not excluded and will be treated as SSH server, but as it does not have address and other properties, an error will be raised. My modification is simply avoiding checking 'local' again in the SSH check.

@alongd
Copy link
Member

alongd commented Jun 4, 2019

Thanks, I think the code looks good, just some comments regarding the commits:

  1. The iPython notebook change should be in its own separate commit, not joint with the local server check
  2. Please rename the Update the rmg_function name: same_species_list. commit message as Updated isomorphic species check method name in rmgdb (since rmg_function with an underscore seems like a real function name)
  3. Generally, commit messages should be at a maximal length of one line, then further description can follow after a line break, to avoid getting:
 Update the function name in notebook and avoid local server check err…

…or in ESS diagnostics```

@xiaoruiDong
Copy link
Contributor Author

xiaoruiDong commented Jun 4, 2019

@alongd Do you think these changes in commits are slightly better? Thanks.

@alongd
Copy link
Member

alongd commented Jun 4, 2019

Looking great! Let's wait for the tests and get the fixes in.
Thanks!

@alongd alongd merged commit f893b48 into master Jun 4, 2019
@alongd alongd deleted the debug_isomorphic branch June 4, 2019 16:33
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

2 participants