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

Create scripts to create images of our major visuals #46

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

pneumatick
Copy link
Contributor

No description provided.

@pneumatick pneumatick changed the title Create a script to hold and create our major visuals Create scripts to create images of our major visuals Aug 21, 2024
@pneumatick pneumatick marked this pull request as ready for review August 21, 2024 17:10
Copy link
Member

@cpaniaguam cpaniaguam left a comment

Choose a reason for hiding this comment

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

Good. Left a few questions and suggestions.

src/plots/pyramid_height_bar.py Outdated Show resolved Hide resolved
'''

# Import the pyramid dataset
df = pd.DataFrame(pd.read_csv("assets/normalized_pyramid_data.csv"))
Copy link
Member

Choose a reason for hiding this comment

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

Convert to CLI and use this as a default argument for the source file?

src/plots/pyramid_height_bar.py Outdated Show resolved Hide resolved
src/plots/pyramid_height_bar.py Outdated Show resolved Hide resolved
src/plots/pyramid_height_bar.py Outdated Show resolved Hide resolved
Comment on lines 70 to 74
'''
Reshape queen data from wide to long (Binary categories get put into a new
column, each category applied to a specific queen given a row, with the status
of that category in another column)
'''
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain the reasoning for using docstrings as sort of comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was simply using them as multiline comments for instances such as this, and to add an element of visual distinctness to separate functionally different portions of each script (specifically the data manipulation portions and the plot construction portions). My understanding was that it's acceptable to use them this way when they aren't at the top of a file or function, but after reviewing the PEP8 style guide, it seems that # comments are preferred, so I will do the conversion for non-docstring comments.

src/plots/queens_height_scatter.py Outdated Show resolved Hide resolved
Comment on lines 10 to 13
'''
Sort King rows (those with actual values for start_of_reign)
in chronological order.
'''
Copy link
Member

Choose a reason for hiding this comment

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

Make it a comment?

Comment on lines +58 to +65
'''
for i in range(len(starts)):
plt.text(starts.iloc[i] - length.iloc[i]/2,
(levels[i]*1.05)/2,
pyramid_owners.iloc[i],
ha='center',
fontsize = '7')
'''
Copy link
Member

Choose a reason for hiding this comment

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

Delete this?

Copy link
Member

Choose a reason for hiding this comment

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

Abstract all this into a function?

@pneumatick pneumatick requested a review from cpaniaguam August 29, 2024 15:32
@@ -1,7 +1,7 @@
pyramid_owner,pyramid_complex,royal_status,daughter_of,royal_mother_title,likely_wife,wife_title,vizier,regent,relationship_to_king,start_of_reign,end_of_reign,length_of_reign,period,dynasty,title,pyramid_texts,site_or_location,orientation,casing,state_of_completion,superstructure_type,height,length,width,angle,notes
Djoser,Djoser,King,,False,False,False,False,False,,2630.0,2611.0,19.0,Old Kingdom,3.0,Step Pyramid of Djoser,,Saqqara,N-S,Limestone,,Pyramid,59.9,120.0,108.0,74,"15 gates, white limestone casing"
Sekhemkhet,Sekhemkhet,King,,False,False,False,False,False,,2611.0,2603.0,8.0,Old Kingdom,3.0,Unfinished Step Pyramid of Sekhemhet,,Saqqara,N-S,Limestone,Unfinished,Pyramid,70,120.0,120.0,71-75,"niched facade, unfinished, left with rough exterior no smoothed limestone, (Verner mentions limestone casing) , ""probably intended to rise about 70 m (230 ft), in seven steps - higher than Djoser's."" (Lehner, 94)"
Nebka?,Nebka?,King,,False,False,False,False,False,,2649.0,2630.0,19.0,Old Kingdom,3.0,Unfinished Pyramid,,Zawiyet el-Aryan,E-W,Unknown,Unfinished,Pyramid,,200.0,200.0,unknown,debate over whether this is Pyramid of Nebka
Nebka?,Nebka?,King,,False,False,False,False,False,,2649.0,2630.0,19.0,Old Kingdom,3.0,Unfinished Pyramid,,Zawiyet el-Aryan,E-W,Unknown,Unfinished,Pyramid,0.0,200.0,200.0,unknown,debate over whether this is Pyramid of Nebka
Copy link
Member

Choose a reason for hiding this comment

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

Consider adding a comment about the zero measurement.

Copy link
Member

@cpaniaguam cpaniaguam left a comment

Choose a reason for hiding this comment

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

Feel free to merge but there remain a few things that could be improved.

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