recently introduced bug

[Gary Ferland]

The following, a sim I was setting up for yesterday's talk, aborts nearly instantly

set save prefix "crash2"

test

save continuum last units microns ".con"

init "honly.ini"

The error is 

Error: Incorrect number of matches (0) for species 'Fe+'

 [Stop in getSpecies at ../species_pseudo_cont.cpp:58, something went wrong]

This is the simplest example of the problem which came up in another context.  The problem seems to be the honly.ini plus save continuum combination, which has always worked.  

Are there automatic ways to bisect master to detect the commit that caused it?  I also wonder if  it would be convenient to create monthly tags in gitlab to save well defined points in time?

thanks,

Gary

[Marios Chatzikos]

As a quick fix, if you change the order of the last two statements, the sim runs.

There is a statement at parse_save.cpp:2623 to check if iron is on before it adds FeII_bands.ini to the list of bands files to process.  We went over it together in May, if you recall.  Obviously, this check should be done in post-parsing.

--
--
http://groups.google.com/group/cloudy-dev
---
You received this message because you are subscribed to the Google Groups "cloudy-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cloudy-dev+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/cloudy-dev/0282139f-e714-4840-9382-9de194a86ab1n%40googlegroups.com.

[Gary J. Ferland]

thanks - have not had a second to look at it, with the review panel then talk, I am a week behind.  I tripped over it when doing a figure for yesterday's talk.  when it works we can make sure both commands occur in sims in the test suite so we trap that class of problems in the future..

To view this discussion on the web visit https://groups.google.com/d/msgid/cloudy-dev/b329e2cb-b103-81a0-2506-933799f89c74%40gmail.com.

--

Gary J. Ferland
Physics, Univ of Kentucky
Lexington KY 40506 USA
Tel: 859 257-8795
https://pa.as.uky.edu/users/gary

[Marios Chatzikos]

This combination already occurs in the test suite: h_t4_conemis.in and h_t4_conemis_lon.in.  But in the 'right' order, so we don't trip over it.

To view this discussion on the web visit https://groups.google.com/d/msgid/cloudy-dev/CAOxTjFTaFeN856w4Otd4nk2X_T%3DHz-VKGuOgLnH3-MdQ6-cFYw%40mail.gmail.com.

[Gary J. Ferland]

we can swap the order in one to make sure both work

To view this discussion on the web visit https://groups.google.com/d/msgid/cloudy-dev/13fba118-b6fd-57cc-82e8-508e1a200ed4%40gmail.com.

[Marios Chatzikos]

The attached patch fixes the problem with the order of commands.

The problem was that the check in parse_save.cpp:2623 against iron being active is done prematurely, when the commands are reversed.  The check could be done from within SpeciesBandsCreate(), but that function also needs to be able to check if a molecule is active, as well, since this is a possible use case.  So, I've defined a function, isSpeciesActive(), that tells if a species (atomic/ionic or molecular) is active or not, and loads bands only if so.  For the function to operate, however, all atomic data must first be loaded, so SpeciesBandsCreate() (and SpeciesPseudoContCreate() with it, for symmetry) has been bumped from InitSimPostparse() to cloudy().

I think this is a robust solution.  The sim that Gary posted no longer fails and produces the continuum file, as expected.  The test suite also passes clean.

Let me know what you think.

Thanks,

Marios

To view this discussion on the web visit https://groups.google.com/d/msgid/cloudy-dev/CAOxTjFTBfg%2BfNOskk8CU%3DYcTG8F2oB1cr-Lb5KwPgLs6cAFCng%40mail.gmail.com.

[Gary J. Ferland]

thanks!  Will study and get back later this afternoon.

To view this discussion on the web visit https://groups.google.com/d/msgid/cloudy-dev/f575ecc4-1b25-385b-92eb-fe067430b5eb%40gmail.com.

[Marios Chatzikos]

As Gary and I realized on Monday, there is a logical error in the way Fe II bands are treated.  The problem is that the bands are processed only when a 'save' command is issued.

The attached patch contains a refactoring that consolidates two distinct, but related, classes, one of which (save_species_bands) was stored on t_save.  A new class is now defined, species_band, which holds the data of save_species_bands, and also an object of the band_emission class (has-a relationship), which is responsible for carrying out the accumulation of the emission in the various bands (NB on master this is named 'species_bands').

save_species_bands has been removed.  Code relevant to adding bands read from the 'save species bands' command to the processing list have been moved from parse_save.cpp to species_pseudo_cont.cpp.  Importantly, adding FeII_bands.ini has been moved from ParseSave to SpeciesBandsCreate.  A boolean field is also added to band_emission to distinguish species disabled in the sim from those disabled in the zone (too low a density) -- not accounting for this distinction blows up the LineStack.  Finally, SaveSpeciesBands now prints an explanatory message in the output file, if the relevant species is not active.

Overall, I think the code is now easier to understand and change.

In terms of behavior, FeII bands always show up in the .out file, if 'print line faint off' is given, and the species is active.  An output file is written only when the 'save species bands' command is also given.

To test the code, you need to apply the patch on the pseudo branch.  The attached tests successfully exercise four cases I could think of:

1) default - bands written in .out, no output file

2) default, save bands - bands written in .out, output file created

3) H only - no bands written in .out, no output file

4) H only, save bands - no bands written in .out, output file created with an apologetic message

The test suite runs clean with the patch.

Let me know what you think.

Marios

To view this discussion on the web visit https://groups.google.com/d/msgid/cloudy-dev/CAOxTjFR9fvyUGSa90fW4Pfmo_OGa1pKGhpLs4o%2BTe%3DADSCtwVg%40mail.gmail.com.

[Marios Chatzikos]

FYI, I've made a few more changes, and pushed the commit to nublado.

Let me know if you have any comments.

-m