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

feat: Raise error for unknown keywords #632

Merged
merged 33 commits into from
Jun 7, 2024

Conversation

MRVermeulenDeltares
Copy link
Contributor

@MRVermeulenDeltares MRVermeulenDeltares commented Apr 24, 2024

#622

  • Throw error when unknown keyword is located in the mdu
  • When multiple unknown keywords are located in the mdu, have the error contain multiple unknown keywords.

@MRVermeulenDeltares MRVermeulenDeltares changed the title feat: give-warning-for-unknown-keyword feat: give-error-for-unknown-keyword May 10, 2024
@MRVermeulenDeltares
Copy link
Contributor Author

MRVermeulenDeltares commented May 10, 2024

After the update to throw an error on an unknown key multiple test started to fail.
I have done some reasearch to try and figure out why.
I came to a few conclusions:

  1. Unknown keywords in the testfiles
  2. Outdated keywords in the testfiles
  3. Not defined keywords in the models (those keywords are in the manual)
  4. Problems with subfiles being validatated and not containing properties in their classes that are defined in their files. (e.g. .bc files and crosssection .ini related files, might be more. )

This is what I have found in regard to the keywords in relation with the tests, I tried to tackle all keywords but I am not 100% sure I have them all.
Manual I used for verification is: content.oss.deltares.nl/dhydro/D-Flow_FM_User_Manual.pdf

Section Keyword defined in manual Mentioned in #634
General guiversion Yes, Table A.1
Geometry pipefile No Yes
Geometry branchfile No
Geometry onednetworkfile No
Geometry shipdeffile No Yes
Geometry bedlevelfile Yes, Table A.4, deprecated and removed
VolumeTables usevolumetablesfile No
Numerics jasfer3d No Yes
Numerics jarhoxu No
Numerics vertadvtypmom3onbnd No
Numerics jposhchk No Yes
Numerics newcorio Yes, Table A.3, research keyword Yes
Numerics jaorgsethu Yes, but only in examples, not in a table defining the keyword
Numerics jaupwindsrc No Yes
Numerics eddyviscositybedfacmax No Yes
Numerics icoriolistype Yes, Table A.3, research keyword Yes
Numerics zlayercenterbedvel Yes, but only once in a text: "10.3 Z-layer modelling" Yes
Numerics epshstem No Yes
Numerics zwsbtol No Yes
Numerics horadvtypzlayer Yes, Table A.3, research keyword Yes
Numerics corioadamsbashfordfac Yes, Table A.3, research keyword Yes
Numerics drop3d Yes, Table A.3, research keyword Yes
Numerics transporttimestepping Yes, Table A.4, deprecated and removed
Numerics transportmethod Yes, Table A.4, deprecated and removed
Numerics noderivedtypes No
Numerics fixedweirfrictscheme No Yes
Numerics jbasqbnddownwindhs Yes, Table A.3, research keyword Yes
Numerics logprofkepsbndin Yes, Table A.3, research keyword Yes
Physics selfattractionloading Yes, But only mentions in Table F.4 Yes
Physics soiltempthick No Yes
Physics jadelvappos No
Physics uniffrictcoef1d2d No Yes
Physics umodlin No
Physics effectspiral Yes, but only in examples, not in a table defining the keyword
Time timestepanalysis No Yes
Time dtfacmax No Yes
Time autotimestepdiff No
Wind windhuorzwsbased Yes, Table A.3, research keyword Yes
Waves wavenikuradse No
Output writepart_domain Yes, but only once in a text: "6.4.2 Partitioning a model" Yes
Output wrimap_salinity ~Yes, Table F.5
Output velocitydirectionclassesinterval No Yes
Output timesplitinterval No Yes
Output wrimap_temperature ~Yes, Table F.5
Output wrimap_input_dt No
Output writebalancefile Yes, Table A.4, deprecated and removed
Output wrihis_heatflux No
Output wrirst_bnd No Yes
Output writedfminterpretedvalues No Yes
Output s1incinterval No
Output velocitymagnitudeclasses No Yes

Action points for issue

  • Wait on implementation of Add mdu keywords (not listed in appendix A of the manual) #634
  • Determine if this list ^ contains keywords that need to be added which aren't defined in another issue
  • Remove unknown keywords from testfiles
  • Further investigate and resolve the issue with .bc files and crosssection .ini related files

@tim-vd-aardweg tim-vd-aardweg changed the title feat: give-error-for-unknown-keyword feat: Raise error for unknown keywords May 13, 2024
@tim-vd-aardweg
Copy link
Contributor

In #634 I wanted to add this test:

    def test_load_model_with_research_keywords_as_fmmodel_raises_error(self):
        input_mdu = (
                test_input_dir / "research" / "mdu_with_research_keywords_from_dia_file_2024.03_release.mdu"
        )

        with pytest.raises(ValueError) as e:
            _ = FMModel(filepath=input_mdu)

        expected_error = "Unknown keywords are detected in section"
        assert expected_error in str(e.value)

But it currently fails, since we have Extra.ignore. So I can't yet add that test. That test can only be added after this issue is implemented.

@tim-vd-aardweg
Copy link
Contributor

This doesn't work for unknown sections, right? Should you create a follow-up issue for this?

Copy link

sonarcloud bot commented Jun 7, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@tim-vd-aardweg tim-vd-aardweg merged commit b7dc73a into main Jun 7, 2024
11 checks passed
@tim-vd-aardweg tim-vd-aardweg deleted the feat/622-give-warning-for-unknown-keywords branch June 7, 2024 09:43
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.

When there are unknown keywords in the mdu, an error should be given instead of them being silently dropped.
2 participants