-
Notifications
You must be signed in to change notification settings - Fork 0
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
Added InputParser module #40
Conversation
For some reason there was an error when trying to use |
Interesting note about the first error. Although I defined the function |
module InputParser | ||
|
||
# Julia implicitly numbers Enum items 0,1,2,3,4 (in order) | ||
# I put them in same order as FORTRAN code (i.e. NOPT=0 meant |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are correct that it definitely doesn't matter. We never want to look at the integer value of an Enum.
return currentLineData | ||
end | ||
|
||
# TODO: Search for error handling (throws Exception/ try-catch block) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we want to be as robust as possible. We should eventually add test cases that verify that the error handling works. We will also be adding constraints on the inputs, since some of them will have a range of physically meaningful values. (For instance Poisson's ration has to lie between 0.5 and 1.0. Anything outside that range is an error. We should check the validity of the values on input (and we should have test cases to check we implemented everything correctly.)
# struct for InputData (OOP in Julia) | ||
struct InputData | ||
problemName::String | ||
numProblems::Int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does Julia have a natural number type? (It probably doesn't, but sometimes when we say integer, we really mean a natural number (or whole number), since negative values don't make sense.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is type UInt
, I can change all counter variables to UInt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, lets use an unsigned int when the data cannot take a negative value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The UInt
seems to print hex values by default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, just go back to using integers. I did a bit of a google, and it isn't worth it to have to convert to decimal. We won't actually display these values very often, but it will be confusing when we do. I think uint is added to Julia for saving memory, not to provide an actual natural number type. We might as well do what everybody else does and use integers to represent what we know are natural numbers. 😄
src/InputParser.jl
Outdated
problemName::String | ||
numProblems::Int | ||
model::Model | ||
foundation::Bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the use of a bool. What does foundation = True mean? I'm wondering if this is a case where it is an enum with two choices, or legitimately a Boolean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is an enum with two choices. Currently true = rectangular slab foundation, false = long strip footing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I change it to an enum? It was a bit unclear since your first sentence seemed to like it being a boolean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on your answer to my question, yes, you should change it to an enum. I do like seeing a Boolean, because so many programmers seem to avoid them, especially in research software. I liked seeing something represented as a Boolean, but then when I thought about it, I had to question whether this was actually a Boolean. 😄 We might add a different type of foundation in the future, and then a Boolean would be problematic. An enum is also better for self-documenting the code. Otherwise you have to remember whether True is rectangular or long strip.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good @EmilSoleymani, but I do have a few questions:
- How can I test your code? I would like to test code before I accept a pull request. I can run the InputParser module at the prompt via:
julia InputParser.jl
. The lack of output lets me know there are no syntax errors, but I don't actually know if it worked. What did you do to convince yourself of correctness? I also triedjulia vdisp.jl
. The code has print statements, but nothing was printed to my output. (I know you are planning on adding test cases later, but I'd like something (even something ad hoc) to help me with the review now.) - It isn't a priority now, but we'll need documentation in the files that will automatically be added to the web-page. You should get in the habit of incorporating some minimal header information in each code file, including the author's name, last date of revision and the module's purpose.
- I wonder if we should split
InputParser.jl
into two modules. One to read the input from a file and another class that creates instances ofInputData
. The struct where you define the input data is encapsulated with reading the file, but we might get our input data from another source. (This is why I like to talk about likely changes.) 😄 We could have a graphical interface that gives us a new instance of InputData. We could also have a program that changes InputData systematically. Maybe we modify the size of the footings as part of an optimization algorithm? It would be good to be able to do this. - How does Julia handle changing state variables? I know they are tedious, but I prefer setters and getters to violating information hiding. (I really should look at the Julia sources you listed in the wiki; I just haven't had time.)
- It is fine to have the hidden input file right now, but in the long term, the input filename would likely be something that the user selects, rather than a "hard coded" filename.
How do I run |
The problem is obviously about importing vdisp. Do I have to do this manually with the package manager before running runtests? (Again, I probably should look at the Julia material, but at the moment, it is quicker to ask you.) 😄 |
Yes, I can run the code now by following your last advice. Thank you. |
if size(currentLineData)[1] < items | ||
println("Error: Invalid input file!") | ||
println("\t>Line $(index): $(items) values expected") | ||
return -1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to raise an Exception here, rather than use a convention that returning -1 means something went wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I plan on learning proper error handling / exception throwing soon. Part of the TODO comments. Would you like that as the step?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, you don't have to add exception handling right now. Rather than just relying on the TODO comment though, please create an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new issue added: #41
Commenting on adding getters and setters. Adding keyword module CircleModule
include("./Point.jl")
using .PointModule
mutable struct Circle
r::Float64
center::Point
Circle(r::Float64, center::Point) = new(r,center)
end
intersects(a::Circle, b::Circle) = (dist(a.center, b.center) <= a.r + b.r)
A = Circle(1.5, Point(1.2, 1.5))
B = Circle(0.334, Point(1.9, 1.4))
println(intersects(A, B))
println(getfield(A, :r))
println(setfield!(A, :r, 5.4))
end
|
Further commenting on getters and setters provided by Julia. There is another pair of functions |
It seems like |
@EmilSoleymani, I am going to accept this PR. The code is working and it is definitely a step in the right direction. Can you please create issues for the comments I made that haven't been addressed, like splitting the module into two distinct modules. I will not delete the branch. If you would rather move to a new branch, you can delete it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned elsewhere, I'll approve this pull request. Please create issues for the outstanding tasks.
InputParser.jl
Added
InputParser
module. Module contains theInputData
struct andModel
enum. Parses over input files (I placed it invdisp/src/.data/input_data.dat
, can be changed of course. It is made to be very robust. Empty lines, extra spaces, extra tabs are all fine as long as all the info expected to be read in a certain line is in the same line. There are some comments labelled as TODO which relate to robustness that will have to be addressed.Vdisp.jl
Added code to include the
InputParser
module and use it to create a newInputData
instance containing the information parsed from file atINPUT_DATA_PATH
.Runtests.jl
Added line that executes the parsing. Added TODO comment reminding us to come up with some test cases for reading inputs, and an idea of how to get started