Skip to content
GitLab
Menu
Projects
Groups
Snippets
Loading...
Help
Help
Support
Community forum
Keyboard shortcuts
?
Submit feedback
Contribute to GitLab
Sign in / Register
Toggle navigation
Menu
Open sidebar
gaoqiong
MIGraphX
Commits
31aed3ec
Commit
31aed3ec
authored
Sep 15, 2023
by
Umang Yadav
Browse files
use threshold instead of tolerance
parent
482e8d61
Changes
13
Hide whitespace changes
Inline
Side-by-side
Showing
13 changed files
with
70 additions
and
35 deletions
+70
-35
docs/driver.rst
docs/driver.rst
+2
-2
examples/migraphx/migraphx_driver/README.md
examples/migraphx/migraphx_driver/README.md
+1
-1
src/driver/main.cpp
src/driver/main.cpp
+5
-5
src/driver/verify.cpp
src/driver/verify.cpp
+8
-8
src/driver/verify.hpp
src/driver/verify.hpp
+3
-3
src/include/migraphx/verify.hpp
src/include/migraphx/verify.hpp
+23
-2
src/include/migraphx/verify_args.hpp
src/include/migraphx/verify_args.hpp
+9
-5
src/verify_args.cpp
src/verify_args.cpp
+12
-2
test/gpu/quantization.cpp
test/gpu/quantization.cpp
+1
-1
test/quantization.cpp
test/quantization.cpp
+2
-2
test/ref/multinomial.cpp
test/ref/multinomial.cpp
+1
-1
test/ref/random_uniform.cpp
test/ref/random_uniform.cpp
+1
-1
test/ref/rnn_ops.cpp
test/ref/rnn_ops.cpp
+2
-2
No files found.
docs/driver.rst
View file @
31aed3ec
...
@@ -50,9 +50,9 @@ Runs reference and CPU or GPU implementations and checks outputs for consistency
...
@@ -50,9 +50,9 @@ Runs reference and CPU or GPU implementations and checks outputs for consistency
.. include:: ./driver/compile.rst
.. include:: ./driver/compile.rst
.. option:: --t
olerance
[double]
.. option:: --t
hreshold
[double]
T
olerance
for error
s
(Default:
8
0)
T
hreshold
for
RMS
error (Default: 0
.001
)
.. option:: -i, --per-instruction
.. option:: -i, --per-instruction
...
...
examples/migraphx/migraphx_driver/README.md
View file @
31aed3ec
...
@@ -55,7 +55,7 @@ See below for a comprehensive list of commands and option arguments, as well as
...
@@ -55,7 +55,7 @@ See below for a comprehensive list of commands and option arguments, as well as
| --exhaustive-tune | Enable exhaustive search to find fastest kernel |
| --exhaustive-tune | Enable exhaustive search to find fastest kernel |
| --fp16 | Quantize for fp16 |
| --fp16 | Quantize for fp16 |
| --int8 | Quantize for int8 |
| --int8 | Quantize for int8 |
| --t
olerance
|
Tolerance
for errors |
| --t
hreshold
|
threshold
for errors |
| --per-instruction
\|
-i | Verify each instruction |
| --per-instruction
\|
-i | Verify each instruction |
| --reduce
\|
-r | Reduce program and verify |
| --reduce
\|
-r | Reduce program and verify |
| --iterations
\|
-n | Number of iterations to run for perf report |
| --iterations
\|
-n | Number of iterations to run for perf report |
...
...
src/driver/main.cpp
View file @
31aed3ec
...
@@ -536,13 +536,13 @@ struct params : command<params>
...
@@ -536,13 +536,13 @@ struct params : command<params>
struct
verify
:
command
<
verify
>
struct
verify
:
command
<
verify
>
{
{
compiler
c
;
compiler
c
;
double
t
olerance
=
8
0
;
double
t
hreshold
=
0
.001
;
bool
per_instruction
=
false
;
bool
per_instruction
=
false
;
bool
reduce
=
false
;
bool
reduce
=
false
;
void
parse
(
argument_parser
&
ap
)
void
parse
(
argument_parser
&
ap
)
{
{
c
.
parse
(
ap
);
c
.
parse
(
ap
);
ap
(
t
olerance
,
{
"--tolerance"
},
ap
.
help
(
"Tolerance for
error
s
"
));
ap
(
t
hreshold
,
{
"--threshold"
},
ap
.
help
(
"threshold for the RMS
error"
));
ap
(
per_instruction
,
ap
(
per_instruction
,
{
"-i"
,
"--per-instruction"
},
{
"-i"
,
"--per-instruction"
},
ap
.
help
(
"Verify each instruction"
),
ap
.
help
(
"Verify each instruction"
),
...
@@ -567,15 +567,15 @@ struct verify : command<verify>
...
@@ -567,15 +567,15 @@ struct verify : command<verify>
if
(
per_instruction
)
if
(
per_instruction
)
{
{
verify_instructions
(
p
,
t
,
c
.
co
,
quantize
,
t
olerance
);
verify_instructions
(
p
,
t
,
c
.
co
,
quantize
,
t
hreshold
);
}
}
else
if
(
reduce
)
else
if
(
reduce
)
{
{
verify_reduced_program
(
p
,
t
,
c
.
co
,
quantize
,
m
,
t
olerance
);
verify_reduced_program
(
p
,
t
,
c
.
co
,
quantize
,
m
,
t
hreshold
);
}
}
else
else
{
{
verify_program
(
c
.
l
.
file
,
p
,
t
,
c
.
co
,
quantize
,
m
,
t
olerance
);
verify_program
(
c
.
l
.
file
,
p
,
t
,
c
.
co
,
quantize
,
m
,
t
hreshold
);
}
}
}
}
};
};
...
...
src/driver/verify.cpp
View file @
31aed3ec
...
@@ -76,7 +76,7 @@ void verify_program(const std::string& name,
...
@@ -76,7 +76,7 @@ void verify_program(const std::string& name,
compile_options
options
,
compile_options
options
,
precision
quantize
,
precision
quantize
,
const
parameter_map
&
inputs
,
const
parameter_map
&
inputs
,
double
t
olerance
)
double
t
hreshold
)
{
{
auto
x
=
run_ref
(
p
,
inputs
);
auto
x
=
run_ref
(
p
,
inputs
);
auto
y
=
run_target
(
p
,
t
,
options
,
quantize
,
inputs
);
auto
y
=
run_target
(
p
,
t
,
options
,
quantize
,
inputs
);
...
@@ -84,7 +84,7 @@ void verify_program(const std::string& name,
...
@@ -84,7 +84,7 @@ void verify_program(const std::string& name,
std
::
size_t
output_num
=
x
.
size
();
std
::
size_t
output_num
=
x
.
size
();
for
(
std
::
size_t
i
=
0
;
i
<
output_num
;
++
i
)
for
(
std
::
size_t
i
=
0
;
i
<
output_num
;
++
i
)
{
{
verify_args
(
name
,
x
[
i
],
y
[
i
],
t
olerance
);
verify_args
(
name
,
x
[
i
],
y
[
i
],
t
hreshold
);
}
}
}
}
...
@@ -92,7 +92,7 @@ void verify_instructions(const program& prog,
...
@@ -92,7 +92,7 @@ void verify_instructions(const program& prog,
const
target
&
t
,
const
target
&
t
,
compile_options
options
,
compile_options
options
,
precision
quantize
,
precision
quantize
,
double
t
olerance
)
double
t
hreshold
)
{
{
const
auto
*
mm_prog
=
prog
.
get_main_module
();
const
auto
*
mm_prog
=
prog
.
get_main_module
();
for
(
auto
&&
ins
:
(
*
mm_prog
))
for
(
auto
&&
ins
:
(
*
mm_prog
))
...
@@ -124,7 +124,7 @@ void verify_instructions(const program& prog,
...
@@ -124,7 +124,7 @@ void verify_instructions(const program& prog,
std
::
cout
<<
"Verify: "
<<
ins
.
name
()
<<
std
::
endl
;
std
::
cout
<<
"Verify: "
<<
ins
.
name
()
<<
std
::
endl
;
std
::
cout
<<
p
<<
std
::
endl
;
std
::
cout
<<
p
<<
std
::
endl
;
verify_program
(
verify_program
(
ins
.
name
(),
p
,
t
,
options
,
quantize
,
create_param_map
(
p
,
false
),
t
olerance
);
ins
.
name
(),
p
,
t
,
options
,
quantize
,
create_param_map
(
p
,
false
),
t
hreshold
);
}
}
catch
(...)
catch
(...)
{
{
...
@@ -140,14 +140,14 @@ void verify_reduced(program p,
...
@@ -140,14 +140,14 @@ void verify_reduced(program p,
compile_options
options
,
compile_options
options
,
precision
quantize
,
precision
quantize
,
const
parameter_map
&
inputs
,
const
parameter_map
&
inputs
,
double
t
olerance
)
double
t
hreshold
)
{
{
auto
*
mm
=
p
.
get_main_module
();
auto
*
mm
=
p
.
get_main_module
();
auto
last
=
std
::
prev
(
mm
->
end
(),
n
+
1
);
auto
last
=
std
::
prev
(
mm
->
end
(),
n
+
1
);
mm
->
remove_instructions
(
last
,
mm
->
end
());
mm
->
remove_instructions
(
last
,
mm
->
end
());
std
::
cout
<<
"Verify: "
<<
n
<<
std
::
endl
;
std
::
cout
<<
"Verify: "
<<
n
<<
std
::
endl
;
std
::
cout
<<
p
<<
std
::
endl
;
std
::
cout
<<
p
<<
std
::
endl
;
verify_program
(
std
::
to_string
(
n
),
p
,
t
,
options
,
quantize
,
inputs
,
t
olerance
);
verify_program
(
std
::
to_string
(
n
),
p
,
t
,
options
,
quantize
,
inputs
,
t
hreshold
);
}
}
void
verify_reduced_program
(
const
program
&
p
,
void
verify_reduced_program
(
const
program
&
p
,
...
@@ -155,14 +155,14 @@ void verify_reduced_program(const program& p,
...
@@ -155,14 +155,14 @@ void verify_reduced_program(const program& p,
compile_options
options
,
compile_options
options
,
precision
quantize
,
precision
quantize
,
const
parameter_map
&
inputs
,
const
parameter_map
&
inputs
,
double
t
olerance
)
double
t
hreshold
)
{
{
const
auto
*
mm
=
p
.
get_main_module
();
const
auto
*
mm
=
p
.
get_main_module
();
auto
n
=
std
::
distance
(
mm
->
begin
(),
mm
->
end
());
auto
n
=
std
::
distance
(
mm
->
begin
(),
mm
->
end
());
std
::
cout
<<
"Verify steps: "
<<
n
<<
std
::
endl
;
std
::
cout
<<
"Verify steps: "
<<
n
<<
std
::
endl
;
for
(
std
::
size_t
i
=
0
;
i
<
n
;
i
++
)
for
(
std
::
size_t
i
=
0
;
i
<
n
;
i
++
)
{
{
verify_reduced
(
p
,
i
,
t
,
options
,
quantize
,
inputs
,
t
olerance
);
verify_reduced
(
p
,
i
,
t
,
options
,
quantize
,
inputs
,
t
hreshold
);
}
}
}
}
...
...
src/driver/verify.hpp
View file @
31aed3ec
...
@@ -37,18 +37,18 @@ void verify_program(const std::string& name,
...
@@ -37,18 +37,18 @@ void verify_program(const std::string& name,
compile_options
options
=
compile_options
{},
compile_options
options
=
compile_options
{},
precision
quantize
=
precision
::
fp32
,
precision
quantize
=
precision
::
fp32
,
const
parameter_map
&
inputs
=
{},
const
parameter_map
&
inputs
=
{},
double
t
olerance
=
100
);
double
t
hreshold
=
0.001
);
void
verify_instructions
(
const
program
&
prog
,
void
verify_instructions
(
const
program
&
prog
,
const
target
&
t
,
const
target
&
t
,
compile_options
options
=
compile_options
{},
compile_options
options
=
compile_options
{},
precision
quantize
=
precision
::
fp32
,
precision
quantize
=
precision
::
fp32
,
double
t
olerance
=
8
0
);
double
t
hreshold
=
0
.001
);
void
verify_reduced_program
(
const
program
&
p
,
void
verify_reduced_program
(
const
program
&
p
,
const
target
&
t
,
const
target
&
t
,
compile_options
options
=
compile_options
{},
compile_options
options
=
compile_options
{},
precision
quantize
=
precision
::
fp32
,
precision
quantize
=
precision
::
fp32
,
const
parameter_map
&
inputs
=
{},
const
parameter_map
&
inputs
=
{},
double
t
olerance
=
8
0
);
double
t
hreshold
=
0
.001
);
}
// namespace MIGRAPHX_INLINE_NS
}
// namespace MIGRAPHX_INLINE_NS
}
// namespace driver
}
// namespace driver
...
...
src/include/migraphx/verify.hpp
View file @
31aed3ec
...
@@ -187,11 +187,32 @@ double rms_range(const R1& r1, const R2& r2)
...
@@ -187,11 +187,32 @@ double rms_range(const R1& r1, const R2& r2)
return
std
::
numeric_limits
<
range_value
<
R1
>>::
max
();
return
std
::
numeric_limits
<
range_value
<
R1
>>::
max
();
}
}
template
<
class
R
>
double
get_threshold
(
const
R
&
,
std
::
size_t
tolerance
=
80
)
{
double
threshold
=
std
::
numeric_limits
<
range_value
<
R
>>::
epsilon
()
*
tolerance
;
return
threshold
;
}
template
<
class
R1
,
class
R2
>
template
<
class
R1
,
class
R2
>
bool
verify_range
(
const
R1
&
r1
,
const
R2
&
r2
,
double
tolerance
=
80
,
double
*
out_error
=
nullptr
)
bool
verify_range
(
const
R1
&
r1
,
const
R2
&
r2
,
std
::
size_t
tolerance
=
80
,
double
*
out_error
=
nullptr
)
{
{
// double threshold = get_threshold(r1, tolerance);
double
threshold
=
std
::
numeric_limits
<
range_value
<
R1
>>::
epsilon
()
*
tolerance
;
double
threshold
=
std
::
numeric_limits
<
range_value
<
R1
>>::
epsilon
()
*
tolerance
;
auto
error
=
rms_range
(
r1
,
r2
);
auto
error
=
rms_range
(
r1
,
r2
);
if
(
out_error
!=
nullptr
)
*
out_error
=
error
;
return
error
<=
threshold
;
}
template
<
class
R1
,
class
R2
>
bool
verify_range
(
const
R1
&
r1
,
const
R2
&
r2
,
double
threshold
,
double
*
out_error
=
nullptr
)
{
auto
error
=
rms_range
(
r1
,
r2
);
if
(
out_error
!=
nullptr
)
if
(
out_error
!=
nullptr
)
*
out_error
=
error
;
*
out_error
=
error
;
return
error
<=
threshold
;
return
error
<=
threshold
;
...
...
src/include/migraphx/verify_args.hpp
View file @
31aed3ec
...
@@ -31,11 +31,15 @@
...
@@ -31,11 +31,15 @@
namespace
migraphx
{
namespace
migraphx
{
inline
namespace
MIGRAPHX_INLINE_NS
{
inline
namespace
MIGRAPHX_INLINE_NS
{
MIGRAPHX_EXPORT
MIGRAPHX_EXPORT
bool
verify_args
(
const
std
::
string
&
name
,
bool
verify_args
(
const
std
::
string
&
name
,
const
argument
&
ref_arg
,
const
argument
&
ref_arg
,
const
argument
&
target_arg
,
const
argument
&
target_arg
,
double
threshold
);
double
tolerance
=
80
);
MIGRAPHX_EXPORT
bool
verify_args
(
const
std
::
string
&
name
,
const
argument
&
ref_arg
,
const
argument
&
target_arg
,
std
::
size_t
tolerance
=
80
);
}
// namespace MIGRAPHX_INLINE_NS
}
// namespace MIGRAPHX_INLINE_NS
}
// namespace migraphx
}
// namespace migraphx
...
...
src/verify_args.cpp
View file @
31aed3ec
...
@@ -30,12 +30,12 @@ inline namespace MIGRAPHX_INLINE_NS {
...
@@ -30,12 +30,12 @@ inline namespace MIGRAPHX_INLINE_NS {
bool
verify_args
(
const
std
::
string
&
name
,
bool
verify_args
(
const
std
::
string
&
name
,
const
argument
&
ref_arg
,
const
argument
&
ref_arg
,
const
argument
&
target_arg
,
const
argument
&
target_arg
,
double
t
olerance
)
double
t
hreshold
)
{
{
bool
passed
=
true
;
bool
passed
=
true
;
visit_all
(
ref_arg
,
target_arg
)([
&
](
auto
ref
,
auto
target
)
{
visit_all
(
ref_arg
,
target_arg
)([
&
](
auto
ref
,
auto
target
)
{
double
error
;
double
error
;
passed
=
verify
::
verify_range
(
ref
,
target
,
t
olerance
,
&
error
);
passed
=
verify
::
verify_range
(
ref
,
target
,
t
hreshold
,
&
error
);
if
(
not
passed
)
if
(
not
passed
)
{
{
// TODO: Check for nans
// TODO: Check for nans
...
@@ -93,5 +93,15 @@ bool verify_args(const std::string& name,
...
@@ -93,5 +93,15 @@ bool verify_args(const std::string& name,
return
passed
;
return
passed
;
}
}
bool
verify_args
(
const
std
::
string
&
name
,
const
argument
&
ref_arg
,
const
argument
&
target_arg
,
std
::
size_t
tolerance
)
{
double
threshold
=
0.001
;
target_arg
.
visit
([
&
](
auto
ta
)
{
threshold
=
verify
::
get_threshold
(
ta
,
tolerance
);
});
return
verify_args
(
name
,
ref_arg
,
target_arg
,
threshold
);
}
}
// namespace MIGRAPHX_INLINE_NS
}
// namespace MIGRAPHX_INLINE_NS
}
// namespace migraphx
}
// namespace migraphx
test/gpu/quantization.cpp
View file @
31aed3ec
...
@@ -118,7 +118,7 @@ TEST_CASE(int8_quantization)
...
@@ -118,7 +118,7 @@ TEST_CASE(int8_quantization)
// the regular pipeline uses the rewrite_quantization in the much
// the regular pipeline uses the rewrite_quantization in the much
// earlier stage.
// earlier stage.
if
(
migraphx
::
gpu
::
mlir_enabled
())
if
(
migraphx
::
gpu
::
mlir_enabled
())
EXPECT
(
migraphx
::
verify
::
verify_range
(
ref_result
,
gpu_result
,
1e5
));
EXPECT
(
migraphx
::
verify
::
verify_range
(
ref_result
,
gpu_result
,
0.0119209
));
else
else
EXPECT
(
migraphx
::
verify
::
verify_range
(
ref_result
,
gpu_result
));
EXPECT
(
migraphx
::
verify
::
verify_range
(
ref_result
,
gpu_result
));
}
}
...
...
test/quantization.cpp
View file @
31aed3ec
...
@@ -83,7 +83,7 @@ TEST_CASE(param_add)
...
@@ -83,7 +83,7 @@ TEST_CASE(param_add)
auto
hs
=
mm
->
add_instruction
(
migraphx
::
make_op
(
"add"
),
hp1
,
hp2
);
auto
hs
=
mm
->
add_instruction
(
migraphx
::
make_op
(
"add"
),
hp1
,
hp2
);
auto
fs
=
mm
->
add_instruction
(
auto
fs
=
mm
->
add_instruction
(
migraphx
::
make_op
(
"convert"
,
migraphx
::
make_op
(
"convert"
,
{{
"target_type"
,
migraphx
::
to_value
(
migraphx
::
shape
::
float_type
)}}),
{{
"target_type"
,
migraphx
::
to_value
(
migraphx
::
shape
::
float_type
)}}),
hs
);
hs
);
if
(
add_return
)
if
(
add_return
)
{
{
...
@@ -1077,7 +1077,7 @@ TEST_CASE(int8_quantization_dot)
...
@@ -1077,7 +1077,7 @@ TEST_CASE(int8_quantization_dot)
std
::
vector
<
float
>
no_quant_result
;
std
::
vector
<
float
>
no_quant_result
;
run_prog
(
p
,
ref_t
,
m
,
no_quant_result
);
run_prog
(
p
,
ref_t
,
m
,
no_quant_result
);
EXPECT
(
migraphx
::
verify
::
verify_range
(
quant_result
,
no_quant_result
,
30000
));
EXPECT
(
migraphx
::
verify
::
verify_range
(
quant_result
,
no_quant_result
,
0.003576
));
}
}
}
}
...
...
test/ref/multinomial.cpp
View file @
31aed3ec
...
@@ -78,5 +78,5 @@ TEST_CASE(multinomial_test)
...
@@ -78,5 +78,5 @@ TEST_CASE(multinomial_test)
std
::
transform
(
res_dist
.
begin
(),
res_dist
.
end
(),
res_norm
.
begin
(),
[
&
](
auto
n
)
{
std
::
transform
(
res_dist
.
begin
(),
res_dist
.
end
(),
res_norm
.
begin
(),
[
&
](
auto
n
)
{
return
static_cast
<
double
>
(
n
)
/
res_dist_sum
;
return
static_cast
<
double
>
(
n
)
/
res_dist_sum
;
});
});
EXPECT
(
migraphx
::
verify
::
verify_range
(
norm
,
res_norm
,
100000
));
EXPECT
(
migraphx
::
verify
::
verify_range
(
norm
,
res_norm
,
0.01
));
}
}
test/ref/random_uniform.cpp
View file @
31aed3ec
...
@@ -68,7 +68,7 @@ TEST_CASE(random_uniform_test)
...
@@ -68,7 +68,7 @@ TEST_CASE(random_uniform_test)
std
::
uniform_real_distribution
<>
dis
(
0.0
,
1.0
);
std
::
uniform_real_distribution
<>
dis
(
0.0
,
1.0
);
std
::
vector
<
float
>
rand_samples
(
sample_size
);
std
::
vector
<
float
>
rand_samples
(
sample_size
);
std
::
generate
(
rand_samples
.
begin
(),
rand_samples
.
end
(),
[
&
]()
{
return
dis
(
gen
);
});
std
::
generate
(
rand_samples
.
begin
(),
rand_samples
.
end
(),
[
&
]()
{
return
dis
(
gen
);
});
EXPECT
(
migraphx
::
verify
::
verify_range
(
result_vec
,
rand_samples
,
100
));
EXPECT
(
migraphx
::
verify
::
verify_range
(
result_vec
,
rand_samples
,
0.00001
));
}
}
TEST_CASE
(
random_uniform_int_test
)
TEST_CASE
(
random_uniform_int_test
)
...
...
test/ref/rnn_ops.cpp
View file @
31aed3ec
...
@@ -1008,7 +1008,7 @@ TEST_CASE(rnn_fp16)
...
@@ -1008,7 +1008,7 @@ TEST_CASE(rnn_fp16)
std
::
vector
<
float
>
last_output_data_gold
{
std
::
vector
<
float
>
last_output_data_gold
{
0.2935145
,
-
0.23719997
,
-
0.31123261
,
-
0.18357255
,
0.
,
0.
,
0.
,
0.
};
0.2935145
,
-
0.23719997
,
-
0.31123261
,
-
0.18357255
,
0.
,
0.
,
0.
,
0.
};
EXPECT
(
migraphx
::
verify
::
verify_range
(
last_output_data
,
last_output_data_gold
,
5e4
));
EXPECT
(
migraphx
::
verify
::
verify_range
(
last_output_data
,
last_output_data_gold
,
0.005
));
}
}
TEST_CASE
(
gru_forward
)
TEST_CASE
(
gru_forward
)
...
@@ -2983,7 +2983,7 @@ TEST_CASE(gru_fp16)
...
@@ -2983,7 +2983,7 @@ TEST_CASE(gru_fp16)
-
0.3969709
,
0.43360898
,
0.35775262
,
0.23280787
,
-
0.52179873
,
-
0.3969709
,
0.43360898
,
0.35775262
,
0.23280787
,
-
0.52179873
,
-
0.21944991
,
0.4535257
,
-
0.13735442
,
0.51757574
,
0.50380427
};
-
0.21944991
,
0.4535257
,
-
0.13735442
,
0.51757574
,
0.50380427
};
EXPECT
(
migraphx
::
verify
::
verify_range
(
hs_data
,
hs_data_gold
,
5e4
));
EXPECT
(
migraphx
::
verify
::
verify_range
(
hs_data
,
hs_data_gold
,
0.00596
));
}
}
TEST_CASE
(
lstm_forward
)
TEST_CASE
(
lstm_forward
)
...
...
Write
Preview
Markdown
is supported
0%
Try again
or
attach a new file
.
Attach a file
Cancel
You are about to add
0
people
to the discussion. Proceed with caution.
Finish editing this message first!
Cancel
Please
register
or
sign in
to comment