search

juami / pytentiostat

python code for the JUAMI potentiostat
BSD 3-Clause "New" or "Revised" License
9 stars 13 forks source link

Experiment setup #157

Closed mspence2 closed 4 years ago

mspence2 commented 4 years ago

This PR adds more detail to the experiment setup section. It still needs two diagrams, but the content is ready for review and comments. Closes #156, #159

mspence2 commented 4 years ago

closes #156

aplymill7 commented 4 years ago

Two things I noticed off the bat are:

  1. Point of Counter electrode not really apparent in initial description (complete circuit, facilitate counter reaction).

  2. Didn't see SHE or SDS defined. SHE might also need explanation too and SDS could provide a site that hosts SDS's maybe.

codecov-io commented 4 years ago

Codecov Report

Merging #157 into master will not change coverage by %. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #157   +/-   ##
=======================================
  Coverage   72.34%   72.34%           
=======================================
  Files           8        8           
  Lines         282      282           
=======================================
  Hits          204      204           
  Misses         78       78

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 5013d07...ca22db7. Read the comment docs.

sbillinge commented 4 years ago

Love the commit messages! 👏

On Sat, Apr 4, 2020 at 12:41 PM Codecov notifications@github.com wrote:

Codecov https://codecov.io/gh/juami/pytentiostat/pull/157?src=pr&el=h1 Report

Merging #157 https://codecov.io/gh/juami/pytentiostat/pull/157?src=pr&el=desc into master https://codecov.io/gh/juami/pytentiostat/commit/5013d0771d0e5a2f6258ed6bbef903c09a527161&el=desc will not change coverage by %. The diff coverage is n/a.

[image: Impacted file tree graph] https://codecov.io/gh/juami/pytentiostat/pull/157?src=pr&el=tree

@@ Coverage Diff @@

master #157 +/-

=======================================

Coverage 72.34% 72.34%

=======================================

Files 8 8

Lines 282 282

=======================================

Hits 204 204

Misses 78 78

Continue to review full report at Codecov https://codecov.io/gh/juami/pytentiostat/pull/157?src=pr&el=continue.

Legend - Click here to learn more https://docs.codecov.io/docs/codecov-delta Δ = absolute (impact), ø = not affected, ? = missing data Powered by Codecov https://codecov.io/gh/juami/pytentiostat/pull/157?src=pr&el=footer. Last update 5013d07...aba7c3d https://codecov.io/gh/juami/pytentiostat/pull/157?src=pr&el=lastupdated. Read the comment docs https://docs.codecov.io/docs/pull-request-comments .

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/juami/pytentiostat/pull/157#issuecomment-609055648, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABAOWUI4Z7O2GOSRNA2VD2TRK5PLLANCNFSM4L4KZSXQ .

-- Professor Simon Billinge Columbia University

mspence2 commented 4 years ago

This draft of the experiment setup is complete. If there are no further comments for the figures or content, it is ready to pull.

sbillinge commented 4 years ago

We can set things up as we want in terms of build. It is not normal to put things that are built from a script into the repo, because a simple build can make a huge number of files and git will track, line-by-line, all the changes in every version of the built html files, so it makes the git db large.

Better is to have a release script (we are using rever but we can have it do what we want) and whenever we make the release the documents build and are then put somewhere that is somewhat archival. In principle, the release script could build the whole conda-pack and everything we want into a gzip file that can then be downloaded onto flash drives....

S

On Mon, Apr 6, 2020 at 11:42 AM Jeremy Hitt notifications@github.com wrote:

@jlhitt1993 commented on this pull request.

In docs/experimentsetup.rst:

+The reference electrode does not undergo any reactions, and it maintains a constant potential. The standard hydrogen +electrode (SHE) is based on the formation of hydrogen gas from two protons and two electrons on a platinum electrode in an ideal solution. +The potential at which this reaction occurs is defined as 0 Volts on the scale of reduction potentials, from which other +reactions can be referenced.

I don't think our setup does the build as of now but if you look in the docs folder, there are already HTML files.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/juami/pytentiostat/pull/157#discussion_r404192274, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABAOWUOBBAU57Z3F7G74VDDRLHZ45ANCNFSM4L4KZSXQ .

-- Professor Simon Billinge Columbia University

aplymill7 commented 4 years ago

Okay so maybe let's pull this one in now and then next priority PR is one with sphinx build script? Not sure where best to archive.

mspence2 commented 4 years ago

Deleted outdated files.

closes #159

I am reading about how to delete a folder, and I have learned that folders are treated as directories in Git and empty directories are not tracked. However, I still see the folder when I am on the branch - will it just not be merged into the main repo when it is empty? I'm a little confused by this.

mspence2 commented 4 years ago

Actually, never mind about the folder. When I check the docs folder online there is no html folder, so it automatically doesn't track it since it's empty. If it continues to show up, I guess I can just delete it locally in my file explorer.

This should be ready to merge.

mspence2 commented 4 years ago

closes #159