Skip to content

Introduction to OOP - #65

Merged
j-m-dean merged 16 commits into
pythoninchemistry:masterfrom
RMeli:oopintro
Jun 15, 2020
Merged

j-m-dean merged 16 commits into
pythoninchemistry:masterfrom
RMeli:oopintro

Conversation

@RMeli

@RMeli RMeli commented May 25, 2020

Copy link
Copy Markdown
Contributor

Simple introduction to OOP. This is written with #46 in mind, but can be moved in the advanced topics as well.

Starts addressing #23 and #24.

@arm61
arm61 requested review from alexsquires and j-m-dean May 26, 2020 12:09
@arm61

arm61 commented May 26, 2020

Copy link
Copy Markdown
Contributor

I am crazy busy this whole week, then off on Monday. So if @j-m-dean or @alexsquires have a chance to look at this feel free. If not I can deal with it next week (sorry for the delay @RMeli).

@j-m-dean j-m-dean left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like it. I think it introduces a complex topic, using the foundations already introduced, whilst applying potentially real world examples.

Sorry about there being so many comments. Action or ignore the suggestions below, as you see fit.

Comments/Suggestions:

-Pre-requisites section before notebook begins. It is in the first sentence, but would be useful for stylist consistency.

  • I think in the style guide we set on first person, so “I/we”, instead of second person, “you”. The justification being it is a friendlier tone and makes the student feel guided through the notebooks.

  • I don’t think we have defined “pass” by this point, so a comment just describing why it is there may be useful

  • We have not introduced masks by this point. They may require a brief introduction before use

  • Currently there are no figures. It would be nice to have some figures to break the text up a bit. Maybe an image of acetylene, and a way of visualising a mask and/or a class?

Typos:

  • In the programming jargon —> In programming jargon

  • Therefore should be usually processed together —> therefore should be processed together

  • To represent H2 —> a hydrogen gas molecule, just because H2 was used to represent the second hydrogen previously

  • Can you think a way of tie together —> Can you think of a way of tying together

  • Object allows you to tie together —> Objects allow you to tie together

  • an object (usually called instance in this context) as an house built following said plan —> an object (usually called an instance in this context) as a house built following said plan

  • the init method takes a self as one —> the init method takes self as one

  • the object itself (which is an instance of the class SimpleMolecule) as first argument of a method —> the object itself (which is an instance of the class SimpleMolecule) as the first argument of a method

  • The variables stored withing a class —> The variables stored within a class

  • inisialised using the init method. —> initialised

  • This example also clarifies a bit more the meaning of self —> This example helps clarify the meaning of self

  • but being an instance of the Molecule, there —> but being an instance of the Molecule class, there

  • In this section we introduced a lot basic principles —> In this section we have introduced a lot of basic principles

  • The class keyword allows to define a class in Python —> The class keyword allows us to define a class in Python

@RMeli

RMeli commented May 26, 2020 •

Copy link
Copy Markdown
Contributor Author

Thank you for the extensive review @j-m-dean ! Sorry for the many typos, I thought I removed most of them but obviously the spell checker is not working well with Jupyter.

Regarding masks, should we introduce them here or add something to the numpy section?


TODOs:

  • Add pre-requisites section
  • Change to "I/we"
  • Comment on pass keyword
  • Introduce masks with numpy (see above)
  • Add figures
    • Acetylene
    • Class (UML)
    • Masking
  • Fix typos

Additional TODOs:

  • Add Google-style doctsrings; they are introduced in the section about functions.

@RMeli

RMeli commented May 26, 2020

Copy link
Copy Markdown
Contributor Author

@j-m-dean there is only one typo I did not address (see comment below). Please let me know if you do not agree with my explanation and I'll be happy to make the suggested change.


Therefore should be usually processed together —> therefore should be processed together

I used "usually" because some operations can be performed on only part of the data (i.e. rotating a molecule will involve coordinates only).

@j-m-dean

Copy link
Copy Markdown
Contributor

Ah, I see why you have added the usually now. However, I do not think "therefore should be usually processed together" makes sense. May I suggest: "therefore should be, in most cases, processed together"?

@RMeli

RMeli commented Jun 12, 2020

Copy link
Copy Markdown
Contributor Author

Sorry for the delay on this, I've been very busy lately. I added UML-style images for the classes. This should address all the comments beside masking. However, I think that the concept of masking is better in the section about numpy arrays. What do you think?

@RMeli RMeli mentioned this pull request Jun 12, 2020
@arm61

arm61 commented Jun 15, 2020

Copy link
Copy Markdown
Contributor

Need to merge 7a0ed35 into your code to get it to pass CI btw, see issue #71

I will leave the actual review to @j-m-dean

@j-m-dean

Copy link
Copy Markdown
Contributor

Looks great! Thank you for all the added stuff. I agree, it does not make sense to introduce masks in this section. I'll add in a little section at the bottom of numpy_arrays at some point.

@j-m-dean
j-m-dean merged commit 766d5b5 into pythoninchemistry:master Jun 15, 2020
@RMeli

RMeli commented Jun 15, 2020

Copy link
Copy Markdown
Contributor Author

Sounds good. Thank you for all the valuable feedback @j-m-dean !

@RMeli
RMeli deleted the oopintro branch June 15, 2020 09:47
@RMeli RMeli mentioned this pull request Jun 15, 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.

4 participants