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

Rewriting asciimaps to be functional #1767

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

Conversation

tlimato
Copy link

@tlimato tlimato commented Jul 7, 2024

What is the change?

Added 2 additional files, asciimaps_functional.py and test_asciimaps_functional.py which contain a function-based approach treating an ASCII map as a dictionary comprised of multiple keys with the respective components, rather than using a class-based approach that is used in asciimaps.py and test_asciimaps.py currently.

Additionally, the 2 implementations of the asciimaps utility in gridBlueprint.py have been changed to use the proper function call structure. Currently, the asciiMapFromGeomAndDomain() has not been validated as an appropriate unit test is not contained in test_asciimaps.py.

PyTest passes everything except for asciiMapFromGeomAndDomain()

Why is the change being made?

The problem: an asciimaps object is required to call a function within that class.

After reading the discussion for issue #436, It was deemed appropriate by my academic advisor, Dr. Andrew Kirby to refactor the asciimaps.py and its accompanying unit tests to use a function-based approach rather than the existing class-based approach. This is not a perfect submission, merely a jumping-off point to make asciimaps.py more extensible and usable for the maintainers of this project.

close #436


Checklist

  • The release notes have been updated if necessary.
  • The documentation is still up-to-date in the doc folder.
  • The dependencies are still up-to-date in pyproject.toml.

I forgot to add compatibility for the geometry names provided in geometry.py
I'm unsure how to effectively unit test this or test the function usage in geometry.py
@CLAassistant
Copy link

CLAassistant commented Jul 7, 2024

CLA assistant check
All committers have signed the CLA.

@john-science john-science added the feature request Smaller user request label Jul 8, 2024
@john-science
Copy link
Member

@tlimato Do you know about black and ruff?

We use them for code formatting in ARMI. We have information on using them in the docs: black / ruff

@john-science
Copy link
Member

@tlimato It looks like your unit tests are failing with:

TypeError: asciiMapFromGeomAndDomain() missing 1 required positional argument: 'domain'[info] Failed to
extract font properties from /usr/share/fonts/truetype/noto/NotoColorEmoji.ttf: In FT2Font: Can not load face
(unknown file format; error code 0x2)

You can see the test logs here.

@john-science john-science self-requested a review July 30, 2024 18:40
@john-science john-science changed the title Function Based Asciimaps.py Rewriting asciimaps to be functional Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Smaller user request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clean up asciimaps
3 participants