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

fix to carry over weirs #206

Merged
merged 4 commits into from
Apr 5, 2021
Merged

fix to carry over weirs #206

merged 4 commits into from
Apr 5, 2021

Conversation

krober10nd
Copy link
Collaborator

  • make sure supplementary boundary condition array data are column arrays.

* make sure supplementary boundary condition array data are column arrays.
@WPringle
Copy link
Collaborator

Are the original arrays guaranteed to be row vectors? Why we have a discrepancy here?

@krober10nd
Copy link
Collaborator Author

with all the manipulation in the plus perhaps they become rows somewhere there.

@WPringle
Copy link
Collaborator

Not sure about that. What is the canonical format that we are using? (row or column vector)

@krober10nd
Copy link
Collaborator Author

everything should be column vectors.

@WPringle
Copy link
Collaborator

WPringle commented Mar 30, 2021

Yeah so for some reason you are saying that originally "obj1" has them as a row vector. Was it like this before the call to function? Or did it get changed during plus function?

Need to see where this happened and prevent it there instead.

@krober10nd
Copy link
Collaborator Author

Not sure. I don't see why it's a big deal. Just make sure they're columns by doing (:) when carrying over.

@WPringle
Copy link
Collaborator

Yeah but the transpose operator may make the shape invalid for another test case. Perhaps it was just wrong in your original mesh

@krober10nd
Copy link
Collaborator Author

No, it wasn't wrong it my original mesh. It should be column vectors so this PR forces that to be true. What other operations are your referring to that would make it so-called "invalid"?

@WPringle
Copy link
Collaborator

WPringle commented Mar 30, 2021

You are using transpose operator to force a row vector to a column vector. But it may already be a column vector. So the PR doesn't necessarily force it to be a column vector (it forces a transpose).

@krober10nd
Copy link
Collaborator Author

That's true. So it needs to use a (:).

@krober10nd
Copy link
Collaborator Author

krober10nd commented Mar 30, 2021

Try to plus m4 into m2

m3 = plus(m4,m2,'arb',{'ds',0});

both m4 and m2 have weirs. Specifically m2 the parent has 6 weirs, and m4, the child, has one.

These meshes were created using all the available tools here in om.

edit: I'll send you the file via email. it's too big to upload

@krober10nd
Copy link
Collaborator Author

krober10nd commented Mar 30, 2021

For example in the case I sent to you via email, the logical array jj is 385 entries long

   obj.bd.ibtype = [obj.bd.ibtype ; obj1.bd.ibtype(jj)];

and obj.bd.ibtype is a scalar. When indexing into obj.bd.ibtype(jj) it produces a row array. This obviously does not concatenate.

Perhaps this case was not previously encountered in our works.

… row vector, also correcting this format to a row vector when reading in from a fort.14
@WPringle
Copy link
Collaborator

WPringle commented Apr 5, 2021

Keith, so it seems that make_bc is making the ibtype and nvell arrays on the fly so they are populated as row arrays, i.e. dimensions of [1 x NBOU].
This does happen to be consistent with nbvv having dimensions of [max(nvell) x NBOU].

So the solution is to change the semi-colon when concatenating to just a blank space or a comma rather than transposing.

It so happens that the shape of ibtype and nvell doesn't matter for writing out the mesh. But when reading in a mesh from fort.14 it was assumed to be dimensions [NBOU x 1]. So I made an edit for that too.

@krober10nd
Copy link
Collaborator Author

Yes I had read in the base mesh from a fort14 so perhaps that why this happened. Did the case I sent now work?

@krober10nd
Copy link
Collaborator Author

Okay, I ran the case, everything works. I agree the fix makes sense. Lets merge this.

@krober10nd krober10nd requested a review from WPringle April 5, 2021 02:00
@krober10nd krober10nd merged commit f54a6d9 into Projection Apr 5, 2021
@krober10nd krober10nd deleted the carryoverweirs_fix branch April 5, 2021 14:59
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.

2 participants