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

Clean up the code and add features #75

Open
wants to merge 459 commits into
base: master
Choose a base branch
from

Conversation

edward-bian
Copy link
Contributor

Cleaned up the code and improved existing functions

Oksana Tkach and others added 30 commits July 20, 2016 11:27
Complete test cases and add formatting for author and status.
…ndex spacing error. Increase efficiency of replace_special.py
Add unittest for DLMF preprocessing
Add support for LaTeX comments in math_mode.py.
Copy link
Contributor

@notjagan notjagan left a comment

Choose a reason for hiding this comment

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

Small style comments, mostly on the ReadMe.

@@ -13,7 +13,97 @@ NOTE: The KLSadd.tex file only deals with chapters 9 and 14, as stated in the do

Copy link
Contributor

Choose a reason for hiding this comment

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

Capitalize "python;" change "The updateChapters.py program" to just "updateChapters.py;" change "pdf" to ".PDF."

_______________________________________________________________________________


First: the files
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to a header; that is,

The Files

@@ -13,7 +13,97 @@ NOTE: The KLSadd.tex file only deals with chapters 9 and 14, as stated in the do

NOTE: when you run updateChapters.py or linetest.py you need the KLSadd.tex file, tempchap9.tex, and tempchap14.tex in order to run the program. The program *generates* updated9.tex and updated14.tex files.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is generates italicized?


First: the files

- KLSadd.tex: This is the addendum we are working with. It has sections that correspond with sections in the chapter files, and it contains paragraphs that must be *inserted* into the chapter files. CAN BE FOUND ONINE IN PDF FORM: https://staff.fnwi.uva.nl/t.h.koornwinder/art/informal/KLSadd.pdf
Copy link
Contributor

Choose a reason for hiding this comment

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

As aforementioned with generated, inserted.


- KLSadd.tex: This is the addendum we are working with. It has sections that correspond with sections in the chapter files, and it contains paragraphs that must be *inserted* into the chapter files. CAN BE FOUND ONINE IN PDF FORM: https://staff.fnwi.uva.nl/t.h.koornwinder/art/informal/KLSadd.pdf

- chap09.tex and chap14.tex: these are chapter files. These are LaTeX documents that are chapters in a book. This is a book written by smart math people. But they did some things wrong, so another math person wants to fix them. That math person wrote an addendum file called KLSadd.tex and our job is to pull paragraphs out of KLSadd and insert them at the end of the relevant section in the chapter files. Every chapter is made up of sections and every section has subsections
Copy link
Contributor

Choose a reason for hiding this comment

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

The last sentence has no period


- newtempchap09.tex and newtempchap14.tex these are output files of linetest.py and this is what a standard output looks like. They are no longer up to date for updateChapters as it sorts by subsection, but still serve as good reference

Next: the specifications
Copy link
Contributor

Choose a reason for hiding this comment

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

This should also be a header, and perhaps the Python file names should be sub-headers.

Toward the end of every section in the chapter file, there is a subsection called "References". It basically just contains a bunch of references to sources and other papers. This is where unsorted materials go For example:
**These aren't real sections, obviously, just an example**

in chap09.tex:
Copy link
Contributor

Choose a reason for hiding this comment

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

Capitalization

Each section has subsections like "Hypergeometric Representation", sections in KLSadd with "Hypergeometric Representation" should be seorted there.

Toward the end of every section in the chapter file, there is a subsection called "References". It basically just contains a bunch of references to sources and other papers. This is where unsorted materials go For example:
**These aren't real sections, obviously, just an example**
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is awkward and clunky.


The variables:

These are the variables in the beginning
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a full sentence, no period

@notjagan notjagan self-requested a review January 3, 2017 21:49
@plusparth
Copy link
Contributor

@edward-bian in the future, if there's something in a pull request review that has been requested that you don't feel is necessary, please comment on it to say why it is unnecessary. I'm looking through my old review and there are a few things outstanding, which are unexplained as of now.

@plusparth
Copy link
Contributor

@edward-bian why is it failing a unit test? Either the test or the code is wrong, and it definitely isn't ready to merge if it's failing a unit test

Copy link
Contributor

@plusparth plusparth left a comment

Choose a reason for hiding this comment

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

@edward-bian

A bunch of these are the same issues that were there in the old review. Almost all of them are really quick fixes - do those first if you want. In addition, worry about fixing the unit tests first.

There are some things in here that may seem like I'm asking you to change some things without reason. In the end, if you think that something should not be changed, it is your call. However, if you decide to do this on a commented change, please reply to the comment mentioning why you chose to not make the change.

```
python updateChapters.py
Python updateChapters.py
Copy link
Contributor

Choose a reason for hiding this comment

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

This Python still should not be capitalized.

@@ -1,13 +1,7 @@
__author__ = "Rahul Shah"
__status__ = "Development"

Copy link
Contributor

Choose a reason for hiding this comment

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

Why were these lines removed? It seems that at least the __author__ line should be there instead of the same thing in the docstring comment


entireFile = file.readlines()
for word in entireFile:
index+=1
Copy link
Contributor

Choose a reason for hiding this comment

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

Conventions - put spaces (index += 1)

Copy link
Contributor

Choose a reason for hiding this comment

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

also @edward-bian generally please look through your code for instances of these kinds of comments where I may have missed it. I may not comment on each instance where there should be spaces but it should still be fixed.

@@ -17,151 +11,151 @@
sections = []
mathPeople = []
newCommands = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Conventions - variable names should be underscore separated rather than camel cased

sec += i
sections.append(sec)

entireFile = file.readlines()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is tab length 8 spaces instead of 4?

designator = "9."
elif chapticker2 == 1:
elif chapticker2 == 14:
designator = "14."
Copy link
Contributor

Choose a reason for hiding this comment

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

why not instead of doing the comparison, do designator = str(chapticker2) + "."

Also why is it called chapticker2 instead of just chapticker

:param paragraphs_to_be_added: List containing additions to be added.
:param kls: The addendum that contains sections to be added (not processed).
:param kls_list_all: List of all identified keywords, fed into the chapter sorter.
:param chapticker2: Which chapter is being searched (9 or 14).
Copy link
Contributor

Choose a reason for hiding this comment

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

why is it chapticker2 and not chapticker as it was before? also this should be spaced with underscores.

As above

# get the index
startindex = 9999
for word in addendum:
index += 1
Copy link
Contributor

Choose a reason for hiding this comment

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

As above

if "subsub" in word and index > startindex-1:
klsaddparas.append(index - 1)
if "\subsection*{" in word and index > startindex-1:
klsaddparas.append(index - 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

three of these conditions are exactly the same

if "14." in word:
chap_nums.append(14)
name = word[word.find("{") + 1: word.find("}")]
math_people.append(name + "#")
Copy link
Contributor

Choose a reason for hiding this comment

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

These conditions result in the same thing but the chap_nums part - the rest should not be in the if

@physikerwelt physikerwelt force-pushed the master branch 3 times, most recently from 75e720e to 9a65ba3 Compare July 11, 2017 06:31
@plusparth
Copy link
Contributor

@edward-bian how much progress have you made

@edward-bian
Copy link
Contributor Author

edward-bian commented Nov 3, 2017 via email

@inchkev
Copy link
Contributor

inchkev commented Apr 8, 2019

@edward-bian How much progress have you made

@notjagan
Copy link
Contributor

Progress report?

@HowardCohl
Copy link
Member

Progress report?

Jagan, you're very funny. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants