Project 5 For John Beverly#8
Conversation
|
Hi @dmcnulty27, Thank you for your invitation to review. I was thinking that perhaps we should be inviting people on our teams to review our projects? Part of the reason is that I would not know how to review your project as I am in Group #1! I believe this is how our group is going to do the review process. Let me know what you think. Best, |
Osiyo@JaronJCheung, Please do not hesitate to reach out with questions! Peace, |
@dmcnulty27, ahh, I see! Okay, that makes sense. Bearing that our work is going to be submitted on one repository, I will invite your team to review our work that will be done on my repository. Thanks for informing me of the process! This is helpful! Take care, |
|
|
||
| sh:ascending true ; | ||
|
|
||
| sh:uniqueMembers true . |
There was a problem hiding this comment.
is this doing the same as
sh:minCount 1 ;
sh:maxCount 1 ; ?
There was a problem hiding this comment.
It is saying exactly one
|
|
||
| sh:path ex:addedAt ; | ||
|
|
||
| sh:ascending true ; |
There was a problem hiding this comment.
I didn't see the sh:ascending property myself while going through the w3 guide, would you mind giving me a reference for its usage?
There was a problem hiding this comment.
built new constraints but I am thankful for your comments :)
|
|
||
| sh:datatype xsd:string ; | ||
|
|
||
| Sh:description |
There was a problem hiding this comment.
Is ex:RegistrationDateShape of type string? If so, why? I would have expected it to be of type xsd:date
There was a problem hiding this comment.
built new constraints but I am thankful for your comments :)
| sh:pattern "ex:city|ex:country|ex:continent" ; | ||
|
|
||
| ] . | ||
|
|
There was a problem hiding this comment.
This is very interesting, I like it. Have you considered extending it, e.g. with constraints about properties of "being close to", or "being in the same location as"?
There was a problem hiding this comment.
No, but maybe we should!
| sh:message "Sometimes gender identity must be Male, Female, or Nonbinary." ; | ||
|
|
||
| ] . | ||
|
|
There was a problem hiding this comment.
That's great, we need to update all the rdfs schemas around that are not including non-binary gender roles.
|
|
||
| sh:minLength 3 ; | ||
|
|
||
| sh:message "The album name must be a string of at least 3 characters." ; |
There was a problem hiding this comment.
What about Led Zeppelin I, II, III and IV? What about Ed Sheeran's +, - etc.?
There was a problem hiding this comment.
built new constraints but I am thankful for your comments :)
|
|
||
| sh:node ex:ArtistShape ; | ||
|
|
||
| sh:message "The album artist must be a valid artist object." ; |
There was a problem hiding this comment.
messages here are really helpful
|
|
||
| sh:minLength 3 ; | ||
|
|
||
| sh:message "The artist name must be a string of at least 3 characters." ; ] ; |
There was a problem hiding this comment.
Can't think of any example, but pretty sure someone has called themselves with names shorter than 3 char. Also, are we talking stage names or given names here?
There was a problem hiding this comment.
built new constraints but I am thankful for your comments :)
Stage names
|
|
||
| sh:path schema:genre ; sh:datatype xsd:string ; | ||
|
|
||
| sh:message "The artist genre must be a string." ; |
There was a problem hiding this comment.
Do you allow for people to do multiple genres?
There was a problem hiding this comment.
In this constraint no
built new constraints but I am thankful for your comments :)
| sh:message "The user email must be a valid email address." ; | ||
|
|
||
| ] . | ||
|
|
There was a problem hiding this comment.
This looks good, I have some general concerns that perhaps are not relevant, given that you might have already gone through this with John.
The Project 5 prompt asked you to provide general and useful shapes. I can see for example that the gender shape can be used for people who want to update their rdfs schemas to include constraints that are inclusive towards non-binary people. The first one might be used by people who want to check the dates in their schema and make sure that they don't have more than one, etc. I am not sure how the others do in this respect.
You refer to properties of an ex: URI. Are you going to build example .owl files that use ex: against which you test the shapes? If so, how are the shapes going to be used by people who have other knowledge bases and want to test them against your schemas?
Hope that this is helpful!
Giacomo
There was a problem hiding this comment.
built new constraints but I am thankful for your comments :)
JaronJCheung
left a comment
There was a problem hiding this comment.
Dear Group 2,
Thank you for inviting me to review your Project 5, Task 2! Below are my general thoughts, comments, and suggestions. Please take and leave whatever is helpful/useful.
(1) I really enjoyed reading your ten examples! I think that they satisfy the three criteria that Professor Beverley outlined: (1) useful, (2) general, and (3) novel. (In order to check for the novel criterion, I did a quick search on the DASH library and it seems your ten SHACL shapes are indeed novel!)
(2) I tested each SHACL shape on https://shacl.org/playground/ and each shape returned no errors. Nice job!
(3) I noticed that for some reason SHACL Playground did not pick up the ex: instances for the following SHACL constraints: 3, 9, and 10. For example, for 3 it did not identify/pick out ex:SymmetryShape. I wonder why because it seems to me the constraints are well formed and consistent with the others. I do not think this matters much (maybe it is a minor discrepancy pointing to something I am missing about the constraints formulation).
(4) Perhaps consider adding a brief description of what the constraint tests for, why it is important and useful, and an example of why you would use it. This would set up any SHACL user that stumbles upon your work for success in utilizing them for future endeavors in the SHACL world.
Excellent job in thinking of useful, general, and novel SHACL constraints. These are important and much needed, especially for ontologies!
Please let me know if you have any questions.
Peace,
Jaron
|
Hey Jaron, could you clarify for the problems you found with 9 and 10? We are reviewing everything else to make sure all the constraints are consistent, but if you can provide any error codes, etc, that would be a big help. Thank you for the very helpful comments. Cheers, |
|
|
||
| sh:node ex:RegistrationShape ; | ||
| sh:maxCount 1 ; | ||
|
|
There was a problem hiding this comment.
so we are saying that ex:relation will have exactly one rdf:value, right? In which sense is this asymmetric? In the sense that in that path we will not get any other value? But I am not sure that this works, because then it also stops us from having any other value at all. I.e. say that we have the path "hugging". This is stopping me from hugging or being hugged from more than one person, while what you want to say is that no one can hug me back (sad), but you don't want to prevent me from hugging more than one person.
There was a problem hiding this comment.
Great point, We noticed this in our meeting on Tuesday that for a proper check of asymmetry (as well as our other relations) we will need at least 4-5 such that minCount:1 and maxCount:5
I appreciate your watchful eye and diligence!
| sh:property [ | ||
|
|
||
| sh:path ex:lineRegistration ; | ||
| sh:path rdf:value ; |
There was a problem hiding this comment.
Interesting, I have never seen rdf:value, what does it do? Also, can we have sh:path without sh:property before it?
There was a problem hiding this comment.
Osiyo Giacomo, thank you for your comment. Regarding the first, rdf: value is used as a path to specify a constraint on the value of the property of the resource the shape applies to. For our constraint, this ensures that every instance of the relation class has exactly one value for the property rdf:value in a string datatype. Additionally, our min/maxCount ensures exactly one value property for each instance of the relation class. Such that our constraint enforces that every instance of relation has a single string value assigned to its property.
Regarding your second comment, I see sh:path after the sh;property on my end so maybe something else that is strange is happening.
Thank you for your review notes :)
There was a problem hiding this comment.
"...has exactly one value": Shouldn't it be "sh:maxCount 1" (instead of "sh:maxCount 2") to ensure this condition?
| sh:maxCount 1 ; | ||
|
|
||
| sh:pattern "[1-12]/[1-31]/[1990-2020]”" ; | ||
| sh:datatype xsd:string |
There was a problem hiding this comment.
why do you need asymmetry to be restricted to strings?
There was a problem hiding this comment.
This constraint is for symmetry but your question extends to both. We restricted both asymmetry and symmetry to strings so that they can be used to represent relationships between different entities in a text format. Additionally restricting strings simplifies the validation process such that, most data types need to be validated and it is easier to achieve this when all the values are strings. However, the restriction to strings is specific to particular constraints, and the reason for string or other restrictions may depend on the data being validated. So, in some cases, the data being validated may not do well to be restricted to strings.
Thanks you for your comment :)
There was a problem hiding this comment.
I am thinking a possibility: A domain ontology has some instances which have not string titles, but those instances (expressed by URIs) can have some symmetric relationship. How does your Symmetry shape validate this possibility? The same comment applies to the above Asymmetry shape.
HI @peihongx Then one would change the constraint to check for specifics of that domain ontology. Our constraints are just meant to be a general, novel, and useful structure that others could utilize for their purposes! Hope that helps :)
| sh:minCount 1 ; | ||
|
|
||
| sh:name "RegistrationDate" ; | ||
| sh:maxCount 1 ; |
There was a problem hiding this comment.
I am very tired, but isn't this the same constraints you put in the previous?
There was a problem hiding this comment.
I can see how you thought that the structures are the same format. I simply have taken a template and adjusted it to a new relation asymmetry above and symmetry here.
Please take care of yourself and get the sleep necessary to power that big ol' brain of yours :) exhaustion is not the key to success! I can speak for all of group 2 when I say your health and wellness are more important than a constraint, project, or task <3
| sh:path rdf:value ; | ||
|
|
||
| ex:RegistrationDateANDlineRegistrationOrder | ||
| sh:minCount 2 ; |
There was a problem hiding this comment.
Say that I hug five people, but no one hugs me back. Then the relation satisfies mincount 2 but is still not symmetric.
There was a problem hiding this comment.
Great point, thank you.
I think I need to add a maxCount as well. Any further suggestions on how to fix this would be appreciated but don't feel obligated :)
| sh:minCount 1 ; | ||
|
|
||
| sh:path ex:spatialcontainment ; | ||
| sh:maxCount 1 ; |
There was a problem hiding this comment.
So we are saying that this class can have at most one rdf:value, right? How is this entailing reflexivity?
There was a problem hiding this comment.
Great point! I think if I add an inverse relation this may help the issue. Such that the inverse must also check for reflexivity.
Thank you for that helpful note!
| 4th Constraint Begins Below | ||
| sh:path ex:hasType ; | ||
|
|
||
| sh:in ( "symmetric, transitive" ) |
There was a problem hiding this comment.
If I understand this correctly, you are saying that things are either strings of type same/opposite or booleans? I am not sure how this constrains symmetry at a structural level, outside of a specific schema that might be using these specific properties and values
| sh:propertyConstraint [ | ||
|
|
||
| sh:targetClass schema:Person ; | ||
| sh:asymmetrical true ; |
There was a problem hiding this comment.
is this to call upon the shape you previously built?
There was a problem hiding this comment.
Nope, it just says the property must either be ex:irreflexiveProperty or ex:aymmtricalProperty but not both!
I hope that helps :)
| ] . | ||
|
|
||
| sh:path schema:name ; | ||
| ex:irreflexivePropertyShape |
There was a problem hiding this comment.
So you are giving to shapes here, right?
There was a problem hiding this comment.
Not quite, and again I am pretty shaky on this but per my understanding the place of ex:irreflexivePropertyShape here is being used as a path value for the property. The shape definition itself is the sh:irreflexive property being set to true. such that the ClassShape uses the asymmeticalPropertyShape and irreflexivePropertyShape in its disjunct for validating the overall shape.
I am not 100% I described this right or intelligibly but I hope I did and that this helps :)
| ] . | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Hi all, sorry for sending this review late but I missed that you updated the files with this new stuff. I have a general concern, that is, you seem to be using in multiple occasions constrains of the type
sh:property [
sh:path rdf:value ;
sh:minCount 1 ;
sh:maxCount 1 ;
Not sure how this would work in enforcing any logical property for the relation(s) in question, or if it should be repeated in different shapes. I have added some other comments in the text. If I am missing something about the purpose of the shapes or how they work, then perhaps I might suggest to add some explanation about it (of couse, it could also be that I am missing the general sense because I am not sleeping these days). Hope this can be of help!
Giacomo
There was a problem hiding this comment.
Giacomo,
Please please please make getting enough rest a priority for yourself :) you deserve to rest! I appreciated all of your comments and I agree the min/maxCount is a reoccurring issue I will set out to resolve! However, I do want to note that this structure will not disappear as it is important for a constraint (at least ours) to be able to constraint for the number of instances to not be infinite. We are grateful for your time and efforts on our project!
Get some sleep, please!
Peace and wellness,
Group2
Thank you @JaronJCheung for your comments we are working to provide descriptions and examples :) |
|
Hi group 2! Here's my feedback. Please take everything I say with a big grain of salt. I'm no expert: -#1 In the asymmetry constraint's enrollment use case, why do you specify that enrollment relation should only be between students and courses in the same department? Wouldn't it still be asymmetric if it was between students and any course? I'm also not sure why an asymmetric constraint would be violated by the course being outside the department. Wouldn't the relation still be asymmetric, since the student is a member of the course and not vice versa? -#7, I'm not sure how the use-case is checking symmetry. I get transitivity, but these don't look like symmetrical relations to me. If I'm a member of a clan, that doesn't mean the clan is a member of me. Am I missing something obvious? -#8, would it even be possible to have a reflexive but asymmetric relation? If relation R holds between x and itself (reflexive), it would have to hold in the other direction (symmetric). Similarly for # 10, would all the irreflexive relations be asymmetric? |
Hi Finn! I appreciate the time and effort you took to review our work! I will take your comments and make the necessary adjustments :)! |
| b. An example of when to use this constraint would be for tracking enrollment in courses at a university. Such that each node would represent a student of course and each relation represents the enrollment of a student in a course. Enrollment relations should only be allowed between students and courses that are in a department. If a violation of this constraint is detected it would mean that there is an enrollment relation that does not satisfy the requirement of a student enrolling in a course without the course being enrolled in a student. By changing ex:AsymmteryShape to ex:AsymmetryEnrollementShape and following our basic structure. | ||
|
|
||
| c. (a -> b) ^ ~(b -> a) | ||
|
|
There was a problem hiding this comment.
Asymmetry of R should be: aRb → ~bRa (equivalent as: ~aRb ∨ ~bRa), so it is not equivalent to aRb ∧ ~bRa. So this is a logical worry about whether your SHACL shape captures asymmetry, different from Giacomo's.
There was a problem hiding this comment.
Could just be my poor ability to translate into symbolic logic :) I will adjust it. Could explain further this worry outside of the poor translation into symbolic logic?
Thanks Karl!
There was a problem hiding this comment.
So if you draw a truth table, you can find the difference between aRb → ~bRa and aRb ∧ ~bRa (your team's proposal). According to your shape here, aRb and ~bRa must be both satisfied by R, but it is possible that both are not satisfied by R. In that possible case, R may be asymmetric.
There was a problem hiding this comment.
Thank you for the suggestion. When our group gets together today we will address this concern!
|
|
||
| a sh:NodeShape ; | ||
|
|
||
| sh:targetClass ex:relation ; |
There was a problem hiding this comment.
So if R is an asymmetric relation, do you want to treat R as a class by using "ex:relation"? Interesting, but I wonder whether this is always reasonable. After all, in BFO there are many aysmmetric object properties like proper parthood, but there is not always a corresponding BFO class called "proper parthood".
There was a problem hiding this comment.
Sure this is just a basic framework. The goal was for any person to grab these constraints and use them for their needs so there is a possibility one could use this constraint to check for proper parthood. Because our constraints were not supposed to be tied to any specific ontology we just want them to be general enough that anybody could utilize them in their ontologies.
There was a problem hiding this comment.
Let me put another way :) It seems that the purpose of your shape is to check whether object properties in any ontology are asymmetric. But in order to make shape work, you also have to treat those object properties as relation classes in that ontology. Unless a ontology includes not only object properties like R but also relation classes R-ness, your shape cannot work as it intends. So my real concern is about the use of "sh:targetClass". I think you don't need to understand a relation or an object property as a class. You can still understand it as a relation by using "targetSubjectsOf" or "targetObjectsOf" to add some contraints to its relata.
There was a problem hiding this comment.
I see, Thank you for this comment. I was not understanding before (clearly). My initial reason for using relation is because well asymmetry is a relation of sorts. I will put this up to my group to see if we can come to a consensus on how to adjust this while keeping it very general, novel, and useful while still passing though the SHACL playground without error.
There was a problem hiding this comment.
@peihongx how does this look?
@Prefix sh: http://www.w3.org/ns/shacl# .
@Prefix ex: http://example.com/ .
@Prefix rdf: http://www.w3.org/1999/02/22-rdf-syntax-ns# .
ex:SymmetricRelationShape
a sh:NodeShape ;
sh:targetClass ex:Relation ;
sh:property [
sh:path ex:hasSource ;
] ;
sh:property [
sh:path ex:hasTarget ;
sh:minCount 1 ;
sh:maxCount 1 ;
sh:message "A relation must have exactly one target"
] ;
sh:property [
sh:path ex:hasInverseRelation ;
sh:minCount 0 ;
sh:maxCount 1 ;
sh:class ex:Relation ;
sh:node [
sh:property [
sh:path ex:hasSource ;
] ;
sh:property [
sh:path ex:hasTarget ;
sh:minCount 1 ;
sh:maxCount 1 ;
sh:message "An inverse relation must have exactly one target"
] ;
sh:property [
sh:path ex:hasInverseRelation ;
sh:minCount 0 ;
sh:maxCount 1 ;
sh:class ex:Relation ;
sh:node [
sh:property [
sh:path ex:hasSource ;
sh:equals [
sh:path [
rdf:reverseProperty ex:hasTarget
]
]
] ;
sh:property [
sh:path ex:hasTarget ;
sh:equals [
sh:path [
rdf:reverseProperty ex:hasSource
]
]
]
]
] ;
sh:property [
sh:path ex:hasSource ;
sh:equals [
sh:path [
rdf:reverseProperty ex:hasTarget
]
]
] ;
sh:property [
sh:path ex:hasTarget ;
sh:equals [
sh:path [
rdf:reverseProperty ex:hasSource
]
]
]
]
] .
There was a problem hiding this comment.
Happy to know your progress! You know, any novel things are always difficult to propose and defend. Your team is doing something unusual! 👍
There was a problem hiding this comment.
I am not sure if this will work but I hope it can.
There was a problem hiding this comment.
sh:property [
sh:path ex:hasInverseRelation ;
sh:minCount 0 ;
sh:maxCount 1 ;
sh:class ex:Relation ;
sh:node [
sh:property [
sh:path ex:hasSource ;
] ;
Maybe we should use sh:inversePath instead of ex:hasInverseRelation? Similarly, I am wondering whether rdf:reverseProperty can be changed to sh:inversePath
sh:property [
sh:path ex:hasTarget ;
sh:minCount 1 ;
sh:maxCount 1 ;
sh:message "An inverse relation must have exactly one target"
]
This property shape still applies to the original target class, so I'm afraid that what it means is that a relation (but not an inverse relation, as you intend) must have exactly one target.
BTW, I'm not very sure why we wanna add constraints about source or the number of targets.
| sh:property [ | ||
|
|
||
| sh:path ex:lineRegistration ; | ||
| sh:path rdf:value ; |
There was a problem hiding this comment.
"...has exactly one value": Shouldn't it be "sh:maxCount 1" (instead of "sh:maxCount 2") to ensure this condition?
|
|
||
| sh:minCount 1 ; | ||
|
|
||
| sh:maxCount 4 ; |
There was a problem hiding this comment.
So your Symmetry shape basically says: Each instance of ex:relation must have 1-4 values. Why does this capture the fact that a relation is symmetric? Suppose we have a relation called "has ... as car seat". For example, my Focus Car has_car_seat Seat 1. If we suppose the seat number of a car is from 1 to 4, then has_car_seat satisfies your SHACL shape. Nonetheless, it is clearly not symmtric because a car has_car_seat a seat, but a seat cannot has_car_seat a car.
There was a problem hiding this comment.
Hi Karl to your point above about exactly one. That was for an old constraint that we got rid of :) hope that helps!
We chose this value system so that there would be enough instances to check for symmetry. Could you explain further the has_car_seat concern? the SHACL constraints are meant to check that graphs are adhering to specific shapes and I am not sure how one would build a graph for has_car_seat (Although I do not doubt you could be able to build one).
There was a problem hiding this comment.
I guess I am reviewing the newest version up to yesterday? Here is what I reviewed:
ex:SymmetryShape
a sh:NodeShape ;
sh:targetClass ex:relation ;
sh:property [
sh:path rdf:value ;
sh:minCount 1 ;
sh:maxCount 4 ;
sh:datatype xsd:string
] .
You know, this SHACL shape basically says: Oh, this shape applies to ex:relation, and each instance of ex:relation must have several string values, that is all. So my general concern still persists: This shape is so general that it is hard to capture some logical property. Has_car_seat may satisfy your shape (given all cars have 4 seats at most), but it is clearly not symmtric. So this shape cannot distinguish symmetric relations from non-symmetric relations.
There was a problem hiding this comment.
I see. Okay I will try to adjust
| sh:maxCount 1 ; | ||
|
|
||
| sh:pattern "[1-12]/[1-31]/[1990-2020]”" ; | ||
| sh:datatype xsd:string |
There was a problem hiding this comment.
I am thinking a possibility: A domain ontology has some instances which have not string titles, but those instances (expressed by URIs) can have some symmetric relationship. How does your Symmetry shape validate this possibility? The same comment applies to the above Asymmetry shape.
HI @peihongx Then one would change the constraint to check for specifics of that domain ontology. Our constraints are just meant to be a general, novel, and useful structure that others could utilize for their purposes! Hope that helps :)
| sh:datatype xsd:string ; | ||
|
|
||
| ] . | ||
|
|
There was a problem hiding this comment.
If we wanna say Continuant_part_of is reflexive, then a SHACL shape capturing this will be:
#independent continuant (bfo:0000004), continuant part of at some time (bfo:0000176)
ex:mcd-1
a sh:NodeShape ;
sh:targetClass bfo:0000004 ;
sh:rule [
a sh:TripleRule ;
sh:subject :this ;
sh:predicate bfo:0000176 ;
sh:object :this ;
sh:condition [
a sh:TriplePattern ;
sh:subject :this ;
sh:predicate rdf:type ;
sh:object bfo:0000004 ;
] ;
] .
This shape means: If a condition is satisfied (an entity x is an independent continuant), then there will be a triple saying "x continuant_part_of x", showing that continuant_part_of is reflecive when it applies to instances of the class "independent continuant".
There was a problem hiding this comment.
This a great example Karl! However our task halts us from using any existing ontologies like BFO.
There was a problem hiding this comment.
Yep, I just wanna show we can use sh:rule to express reflexivity, so in fact you can change those BFO terms to more general terms and thus get a more general shape for reflexivity.
There was a problem hiding this comment.
OHH I see :)
Thank you for this!
| sh:maxCount 4 ; | ||
|
|
||
| sh:datatype xsd:string ; | ||
|
|
There was a problem hiding this comment.
From my perspective, it seems that what really play a role in your shapes are sh:minCount and sh:maxCount, is this right? Sorry I cannot find other constraints.
|
|
||
| sh:minCount 1 ; | ||
|
|
||
| sh:maxCount 8 ; |
There was a problem hiding this comment.
What does "sh:maxCount 8" mean? Can't it be "sh:maxCount 7"? I'm just wondering how a particular number is specified here.
There was a problem hiding this comment.
I truly just chose some numbers lol as these are meant to be examples
| sh:property [ | ||
|
|
||
| sh:or ( ex:asymmetricalPropertyShape ex:irreflexivePropertyShape ) ; | ||
|
|
There was a problem hiding this comment.
I'm wondering whether we can use "sh:or" like this. sh:or can be used to include several nodes (including blank nodes like "[ sh:path ex:givenName ; sh:minCount 1 ; ]" ), but can it be used to directly include a node shape?
There was a problem hiding this comment.
Yes, you could use sh:or to include several node shapes. The sh:or property takes a list of node shapes as its values and any data that conforms to at least one of the node shapes in the list would be valid. Let's say we have three node shapes ex:Shape1, ex:Shape2, and ex:Shape3 you can use sh:or to combine them like this
ex:CombinedShape
a sh:NodeShape ;
sh:or ( ex:Shape1 ex:Shape2 ex:Shape3 ) .
ex:CombinedShape is a new node shape that includes the three node shapes ex:Shape1, ex:Shape2, and ex:Shape3 as alternatives. Any data that conforms to at least one of these node shapes will be considered valid. The node shapes included in the sh:or list can be simple or complex, and can include blank nodes as well.
I hope this helps :)
There was a problem hiding this comment.
Goood to know about this, thanks!
| 4th Constraint Begins Below | ||
| sh:path ex:hasType ; | ||
|
|
||
| sh:in ( "symmetric, transitive" ) |
It is impossible to have a reflexive but asymmetric relation because asymmetry logically implies irreflexivity. |
Hi @Finn1928 and @peihongx We did get rid of that constraint! Thanks! |
@giacomodecolle
@JaronJCheung
Please review :)
Thank you for your work on this :)