Unit test for stratigraphy column

[xyl96]

We find when we added up three facies height togther from sc.csv, the height is> the height from adm.csv. Can we make a unit test to make sure the numbers in sc.csv agrees with the adm?

[jhidding]

@xyl96 I see in your issue_95 branch that you started fixing the function, but this issue specifically mentions creating a unit test, nothing about fixing yet. I'm deliberate in taking this in small steps. Please let me know if something is unclear.

[xyl96]

Ahh, my bad. I have made a test showing in the exported csv, the sc does not equal to sac when we have multiple facies. Also, I have looked at the code last week.. I think it might be related to the function stratigraphic_column: In this function, we have to calculate the acc based on deposition and disintegration. Then we compare the acc with existing deposition and substract from it. However, in this process, we are now substract it for each facies not for the entire deposition?

[xyl96]

The disagreement between sac and sc is possibly because sac is calculated from state.sediment_height while sc is calculated seperately from deposition and disintegration.

[jhidding]

That last bit shouldn't matter barring some numerical round-off; I just used what was most convenient. The sediment_height can be computed from the deposition and disintegration, but since it is already computed during the model run, it might as well be saved.

I don't understand what you're saying in that first comment. Your language is inexact and unclear. Please give me an example where the current implementation is demonstrably wrong, and then I mean: give me code. You notified me of this issue months ago and I still haven't seen that example. It is not good enough that I know that you looked at something. This issue is still lingering because I haven't been shown where it goes wrong.

Make a minimal example, preferably one-dimensional (set y dimension to 1), where the sac is wrong. If the problem depends on having multiple facies, keep two or three, otherwise reduce to a single facies. You may just paste the code for that example in this issue.

After that I can help you to turn this example into a unit test. This unit-test will first fail on the current code and then we fix the code so the unit-test passes. This way we can be certain that the new code fixes something.

In summary, the time-line is as follows:

1. Write an issue describing the problem in detail and containing an input specification where you think the output is wrong. Preferably the example should take a fraction of a second to run, not minutes.
2. Create a new branch referencing the issue number and also a descriptive few words of the issue. In this case 95-sac-mismatch would be good. (I know we worked with just numbers in the past, but I've since seen the light)
3. Turn the issue into a unit test that fails. Failing unit-tests are the starting point of improvement. Push this unit test to the new branch, so we can all see what's happening.
4. Make the unit test pass. i.e. fix the code.
5. Open a Pull Request. We'll review the test and the code.

In this time-line it is not specified who actually does what. The cool thing is that this could be five different people and none of them need to know too much.

[xyl96]

I think I have to explain a little bit more. Very Sorry for the misunderstanding.

We have the branch issue-95, and now I have changed the name to issue_95_sac_sc_mismatch.

Rewrite the initiative of this issue: the total sediments of stratigraphic column (sc) in single location (e.g., loc1) added together does not equal to the sediments height extracted from sediment accumulation curve (sac).

For example, in the alcap-example.h5, the total sediments of three facies together is 56.42 m, while the height of the sediments from sac is 54.40 m. Therefore, when I run @test sum(sc1_f1+sc1_f2+sc1_f3) ≈ sac1 in TestExport.jl, it gives an error.

I further tested a single facies scenario as an example (single_facies_run.jl). The sum of sc1_f1 is 113m and > the end of sac (54.50m).

I then changed the the acc in the function stratigraphic_column in Export.jl to acc = data.deposition[facies, ts], instead of acc = data.deposition[facies, ts] - data.disintegration[facies, ts], but the resultant sum of sc1_f1 is 72.2 and still > the end of sac (54.50m).

The Test showing the sac-sc-mismatch is in TestExport.jl.

I hope this is more clear this time and I am very sorry for the previous unclear expressions.

[jhidding]

I think you misunderstand: the cumulative sum of the stratigraphic column should equal the age-depth-model, not the sediment accumulation curve. I implemented those functions to specifications given by @NiklasHohmann. I suggest you two start talking about what each of these quantities should mean.

[jhidding]

I created a Draft PR, just so that I could leave some comments on your work so far.