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

Rework Material Data Structures #109

Merged
merged 39 commits into from
Aug 27, 2018
Merged

Rework Material Data Structures #109

merged 39 commits into from
Aug 27, 2018

Conversation

rrsettgast
Copy link
Member

No description provided.

… arbitrary ManagedGroup. Allocated material data off of CellBlockSubRegions.
@rrsettgast
Copy link
Member Author

@klevzoff can you update the single phase flow solver to use the auto-generated fields from the materials, and update single phase flow solver inputs to evaluate?

{
dataRepository::ManagedGroup const * const
constitutiveRelation = constitutiveGroup->GetGroup(matIndex);
accessor[kReg][kSubReg][matIndex].set(constitutiveRelation->getReference<VIEWTYPE>(viewName));
Copy link
Contributor

@klevzoff klevzoff Aug 21, 2018

Choose a reason for hiding this comment

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

Here matIndex is determined by relative ordering of constitutive models in a (sub)region. Does that mean the material index for the same model (e.g. water) can be different between (sub)regions? If so, I can't use this accessor in a solver, since I can't reliably figure out the material index of interest (or I'd have to search for it in every (sub)region whenever I use it). I'd rather grab the field view wrapper directly by its name using the defined naming convention modelName + "_" + fieldName, since modelName is something the solver will know (provided via input).

Copy link
Member Author

Choose a reason for hiding this comment

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

@klevzoff good point. i can fix the accessor to enforce constant index across all regions.

Copy link
Contributor

@klevzoff klevzoff Aug 21, 2018

Choose a reason for hiding this comment

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

@rrsettgast I also just realized, the accessor will likely carry a lot of null pointers wrapped as references. E.g. if I have a fluid model with "fluidDensity" field and a solid model with a "porosity" field, and I request a material accessor for "fluidDensity", which the second model does not have, but it will still be extracted as a nullptr. I'm not going to access that null reference, but the very fact that it exists seems like a trouble waiting to happen. Or technically it's UB i guess.

Copy link
Contributor

Choose a reason for hiding this comment

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

As I suspected, in debug mode this throws SIGSEGV when trying to dereference a nullptr, even though I'm never using the returned reference

Copy link
Member Author

Choose a reason for hiding this comment

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

@klevzoff for what problem does this SIGSEGV?

Copy link
Contributor

Choose a reason for hiding this comment

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

@rrsettgast this was before your commit 3451419
I was working on some changes in parallel (which are now merged with yours and pushed), this particular line was segfaulting on any 2-material test. I don't know if still does, because since today's updates I'm getting errors from ChaiVector (munmap_chunk(): invalid pointer and SIGABRT) during ManagedGroup::Initialize(), so I can't run anything atm. I'll try to debug and document it separately.

@rrsettgast
Copy link
Member Author

@klevzoff It looks like you are doing some rework of the actual constitutive hierarchy...not that there is much of a hierarchy to start with. We should do the actual rework of the constitutive class hierarchy in another PR after this. This really became a PR centered around the interface to the material data, and the application of boundary conditions.

@rrsettgast
Copy link
Member Author

@klevzoff when you move or rename a file, please do a "git mv".

https://stackoverflow.com/questions/2641146/handling-file-renames-in-git

@klevzoff
Copy link
Contributor

@rrsettgast The hierarchy change was just a way to test having two meaningful constitutive models (i.e. such that both are actually used by the solver). The main change was about how the solver interacts with the models: it's now actually making use of the state update function, rather than just extracting the state data and operating it manually.

@klevzoff
Copy link
Contributor

@rrsettgast noted. The IDE is supposed to take care of using appropriate git commands when I do automated refactoring (such as renaming a class), but it seems to miss sometimes.

@rrsettgast
Copy link
Member Author

@klevzoff
The SinglePhaseFlowSolver doesn't appear work anymore. I am trying to finish up the silo output for mixed material zones but I can't verify that I am doing things correct. I noticed that you reworked the set of variables that are used to perform the calculation. I don't really agree with the removal of the delta's in favor of copying the fields at the beginning of the step, so we should have a discussion about that sort of decision. Additionally I think that this sort of thing needs to be done it its own pull request. Unless you have a major objection, I am going to make a branch for these commits off of this branch, rebase them out of this branch, and rebase the commits out the commits that were kept in this branch out of the other branch.

@rrsettgast
Copy link
Member Author

@klevzoff never mind about the rebase. I can't do it. The histories are too divergent to get a successful rebase.

@klevzoff
Copy link
Contributor

klevzoff commented Aug 24, 2018

@rrsettgast I'm getting SIGESGV from here (when running 1D_10_compressible_2material.xml):

__strlen_avx2 0x00007f6f6bcb85a1
DBStringArrayToStringList 0x00007f6f6e00f1be
db_hdf5_PutMultivar 0x00007f6f6e0a81e6
DBPutMultivar 0x00007f6f6dff8678
geosx::SiloFile::WriteMultiXXXX<int (*)(DBfile*, char const*, int, char const* const*, int const*, DBoptlist_ const*)> SiloFile.hpp:1030
geosx::SiloFile::WriteMaterialDataField<double, double> SiloFile.hpp:968
geosx::SiloFile::WriteMaterialMapsFullStorage SiloFile.cpp:1135
geosx::SiloFile::WriteMeshLevel SiloFile.cpp:1649
geosx::SiloFile::WriteDomainPartition SiloFile.cpp:1468
geosx::SiloOutput::Execute SiloOutput.cpp:96
...

At a first glance, seems to be nothing wrong with data that goes in. I can't see into silo functions though.
Maybe I did something wrong (I had to locally merge bugfix/noChaiString and replace a few more array1d<string> to be able to run).

Anyway, if you merge develop into this PR and it works for you, it's good to go (can't approve while conflicting changes exist). Or I can push mine.

@rrsettgast
Copy link
Member Author

resolves #63

@rrsettgast
Copy link
Member Author

resolves #80

@rrsettgast rrsettgast merged commit eac11f3 into develop Aug 27, 2018
@rrsettgast rrsettgast deleted the feature/multiMaterial branch August 31, 2018 04:13
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.

3 participants