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

Removes blocking behavior when trident is not configured #142

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

Conversation

chummels
Copy link
Collaborator

@chummels chummels commented Jun 24, 2020

This PR addresses Issue #57 and makes it so that Trident can operate minimally even when it hasn't been configured. Prior to this, Trident would block and require user input in order to proceed, which meant that it didn't play well with other codes as a dependency or with automatic documentation generation with Readthedocs.

This PR removes that blocking behavior. When initially imported, trident prints a verbose message to the user requesting that they run a command to automatically generate the config file and download the ion_balance file. Furthermore, if this is ignored, Trident tries to operate without this information. If specific functions require ion balance information, it raises an appropriate exception and halts.

This started out as a PR to update the installation documentation, which is why there is a slight update to the docs, but I figured this blocking behavior should be addressed first, and then the rest of the install docs can be updated.

@@ -161,14 +159,6 @@ def make_simple_ray(dataset_file, start_position, end_position,
and :lines:='all', it will add every ion of every element up to Zinc.
Default: None

:ionization_table: string, optional
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed this since I realized it is not actually used at all. I guess we could deprecate and leave in, but I seriously doubt anyone ever manually sets the ionization table when generating a ray and just uses the default setup in their config file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I could see how this could be useful for making comparisons between multiple ionization tables. It's hard to know for sure that no one is using it. I'd say at least it should be deprecated first.

@@ -450,16 +450,3 @@ def make_onezone_ray(density=1e-26, temperature=1000, metallicity=0.3,
ray.domain_right_edge = ray.domain_right_edge.to('code_length')

return ray

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It appears that the astropy error that led to this kluge has been fixed in the last three years. I cannot find the PR that fixed it in astropy, but I can now import trident in its source directory and it doesn't fail out the way it used to. So I'm pulling this.

@coveralls
Copy link

coveralls commented Jun 26, 2020

Pull Request Test Coverage Report for Build 1141

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 97 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.1%) to 75.787%

Files with Coverage Reduction New Missed Lines %
/home/circleci/trident/trident/ray_generator.py 11 88.3%
/home/circleci/trident/trident/spectrum_generator.py 22 84.26%
/home/circleci/trident/trident/ion_balance.py 27 84.06%
/home/circleci/trident/trident/config.py 37 51.52%
Totals Coverage Status
Change from base Build 1066: -0.1%
Covered Lines: 1806
Relevant Lines: 2383

💛 - Coveralls

doc/source/installation.rst Outdated Show resolved Hide resolved
@@ -161,14 +159,6 @@ def make_simple_ray(dataset_file, start_position, end_position,
and :lines:='all', it will add every ion of every element up to Zinc.
Default: None

:ionization_table: string, optional
Copy link
Collaborator

Choose a reason for hiding this comment

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

I could see how this could be useful for making comparisons between multiple ionization tables. It's hard to know for sure that no one is using it. I'd say at least it should be deprecated first.

@chummels
Copy link
Collaborator Author

chummels commented Jul 10, 2020

@brittonsmith I addressed these comments. Is it possible for you to try this out locally to ensure this behaves the way you want it to, since you were the genesis of Issue #57 which spawned this PR.

the development version of Trident requires the development version of yt, both
built from source. Don't worry if you want to change later, you can always
switch between the two versions easily enough by following the directions in
:ref:`uninstallation`.

.. _step-1:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since yt is a hard dependency for trident, it should be possible to just to pip install trident to get stable releases of both codes. It might be useful to add a suggestion to try that before installing yt and trident separately.

Copy link
Collaborator

@brittonsmith brittonsmith left a comment

Choose a reason for hiding this comment

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

Looks good to me. I only had one small comment about installation. I tested importing without a .trident directory and it worked. It might be a good idea to add a test for this, i.e., an additional command in the circleci script that tries to import trident before putting the config file in place.

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

3 participants